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

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

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?

Thanks, Roger.