Re: [Qemu-devel] [PATCH] s390x/event-facility: variable-length event masks

[Cornelia Huck]

On Mon, 16 Oct 2017 13:11:19 -0400
"Jason J. Herne" <address@hidden> wrote:

> On 10/16/2017 09:44 AM, Cornelia Huck wrote:
> > On Wed, 11 Oct 2017 09:39:53 -0400
> > "Jason J. Herne" <address@hidden> wrote:  
> >> ...  
> > 
> > Out of curiousity: Do you have a guest that can verify this for mask
> > lengths != 4? Given that the guest I wrote that one for back then is
> > not publicly available...
> >   
> 
> I do have a guest OS that exercises larger mask lengths. Nothing 
> publicly available however.

Great. I have verified that this works with Linux under tcg as well.

> 
> >> ...
> >>   static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
> >>   {
> >>       WriteEventMask *we_mask = (WriteEventMask *) sccb;
> >> +    uint16_t mask_length = be16_to_cpu(we_mask->mask_length);
> >> +    uint32_t tmp_mask;
> >>   
> >> -    /* Attention: We assume that Linux uses 4-byte masks, what it actually
> >> -       does. Architecture allows for masks of variable size, though */
> >> -    if (be16_to_cpu(we_mask->mask_length) != 4) {
> >> +    if (!mask_length || (mask_length > SCLP_EVENT_MASK_LEN_MAX)) {
> >>           sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH);
> >>           goto out;
> >>       }
> >>   
> >> +    /*
> >> +     * Note: We currently only support masks up to 4 byte length;
> >> +     *       the remainder is filled up with zeroes. Linux uses
> >> +     *       a 4 byte mask length.
> >> +     */  
> > 
> > Do you have any plans for extending this? Or is there no need?
> > 
> > (I have to ask those questions, as the documentation is not publicly
> > available...)
> >   
> 
> As of right now 4B is all that is actually needed for any of the masks. 
> The reason to allow for 32 byte requests is for guest operating systems 
> who are already planning for the future. Someday more than 4B may be 
> used but we have no way to know if or when.

It should not be hard to extend the masks on top of this patch, I
think. Let's see what comes.

> 
> >> ...  
> > 
> > Patch looks reasonable as far as I can see (the documentation issue
> > again...)
> > 
> > Did you need to change much from the original patch?
> >   
> 
> Very little. Just a quick forward fit and test and all is well :)
> Thanks for the original work. This saved me some time.

You're welcome :)