Re: [Qemu-devel] [PATCH RFC 04/11] s390/qemu: cpu model cpu facilitiy support

[Michael Mueller]

On Thu, 03 Oct 2013 07:53:02 -0700
Richard Henderson <address@hidden> wrote:

> On 10/02/2013 04:33 AM, Michael Mueller wrote:
> > +/* set a specific bit in facility set */
> > +static void set_facility(unsigned int nr, void *facilities)
> > +{
> > +    unsigned char *ptr;
> > +
> > +    if (nr >= MAX_S390_FACILITY_BIT) {
> > +        return;
> > +    }
> > +    ptr = (unsigned char *) facilities + (nr >> 3);
> > +    *ptr |= (0x80 >> (nr & 7));
> > +}
> 
> I'd like to see this done in a host endian independent way.

valid point, I will address that.

> 
> See my recent patch set to add facility support to the tcg side
> of target-s390, with which this patch set is going to conflict.

I saw it, that's why I've pushed this patch set out for RFC.

> 
> Is there a good reason not to compute these facility masks at
> compile-time?  See
> 
>  http://patchwork.ozlabs.org/patch/279534/
> 
> where I have pre-computed (possibly incomplete) facilities lists
> for the major cpu revisions.

Your facilities lists have been derived from constants introduced in head.S. 
They represent model
specific "required" facilities. That does not necessarily mean for all of them 
that they have
been introduced with the respective model. Some have been introduced already 
with model: N-1, GA>1

> 
> It just seems like your facility_availability array is the wrong
> way to go about things, taking up more memory and startup time
> than necessary.
> 

The reason why I represent them in the data segment is that they are are not 
considered as
constants in the s390 system perspective. I plan to be able to simulate 
firmware migration that
introduce new facility bits without the need of restarting the guest OS.

A second reason for using 2k of memory here is to fully represent the 
facilities as defined
in the s390x architecture. The SIE state needs it and I want to represent it 
identically in user
space and KVM. Otherwise I would need a specific interface just for the 
facilities.

I will consider to alternatively use your way of FAC definition, but still that 
would include a
copy.

In regard to the startup time, I will figure out what the overhead is.

Thanks a lot!
Michael
> 
> r~
>