Re: [PATCH] x86/vPIC: register only one ELCR handler instance

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, May 30, 2023 at 10:48:02AM +0200, Jan Beulich wrote:
> On 29.05.2023 10:39, Roger Pau Monné wrote:
> > On Fri, May 26, 2023 at 09:35:04AM +0200, Jan Beulich wrote:
> >> There's no point consuming two port-I/O slots. Even less so considering
> >> that some real hardware permits both ports to be accessed in one go,
> >> emulating of which requires there to be only a single instance.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>
> >> --- a/xen/arch/x86/hvm/vpic.c
> >> +++ b/xen/arch/x86/hvm/vpic.c
> >> @@ -377,25 +377,34 @@ static int cf_check vpic_intercept_elcr_
> >>      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> >>  {
> >>      struct hvm_hw_vpic *vpic;
> >> -    uint32_t data;
> >> +    unsigned int data, shift = 0;
> >>  
> >> -    BUG_ON(bytes != 1);
> >> +    BUG_ON(bytes > 2 - (port & 1));
> >>  
> >>      vpic = &current->domain->arch.hvm.vpic[port & 1];
> >>  
> >> -    if ( dir == IOREQ_WRITE )
> >> -    {
> >> -        /* Some IRs are always edge trig. Slave IR is always level trig. 
> >> */
> >> -        data = *val & vpic_elcr_mask(vpic);
> >> -        if ( vpic->is_master )
> >> -            data |= 1 << 2;
> >> -        vpic->elcr = data;
> >> -    }
> >> -    else
> >> -    {
> >> -        /* Reader should not see hardcoded level-triggered slave IR. */
> >> -        *val = vpic->elcr & vpic_elcr_mask(vpic);
> >> -    }
> >> +    do {
> >> +        if ( dir == IOREQ_WRITE )
> >> +        {
> >> +            /* Some IRs are always edge trig. Slave IR is always level 
> >> trig. */
> >> +            data = (*val >> shift) & vpic_elcr_mask(vpic);
> >> +            if ( vpic->is_master )
> >> +                data |= 1 << 2;
> > 
> > Not that you added this, but I'm confused.  The spec I'm reading
> > explicitly states that bits 0:2 are reserved and must be 0.
> > 
> > Is this some quirk of the specific chipset we aim to emulate?
> 
> I don't think so. Note that upon reads the bit is masked out again.
> Adding back further context, there's even a comment to this effect:
> 
> +        else
> +        {
> +            /* Reader should not see hardcoded level-triggered slave IR. */
> +            data = vpic->elcr & vpic_elcr_mask(vpic);
> 
> The setting of the bit is solely for internal handling purposes,
> aiui.

Oh, I see, I should have paid more attention to the "Slave IR is
always level trig.", might have been helpful if this was noted as an
internal implementation detail.

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.