Re: [PATCH v3 7/7] amd/pvh: enable ACPI C1E disable quirk on PVH Dom0

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Nov 02, 2018 at 03:06:58AM -0600, Jan Beulich wrote:
> >>> On 30.10.18 at 16:41, <roger.pau@citrix.com> wrote:
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -44,6 +44,9 @@ integer_param("cpuid_mask_thermal_ecx", opt_cpuid_mask_thermal_ecx);
> >  s8 __read_mostly opt_allow_unsafe;
> >  boolean_param("allow_unsafe", opt_allow_unsafe);
> >  
> > +/* Signal whether the ACPI C1E quirk is required. */
> > +bool amd_acpi_c1e_quirk;
> 
> __read_mostly
> 
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -1273,6 +1273,24 @@ void svm_host_osvw_init()
> >      spin_unlock(&osvw_lock);
> >  }
> >  
> > +static int acpi_c1e_quirk(int dir, unsigned int port, unsigned int bytes,
> > +                          uint32_t *val)
> > +{
> > +    ASSERT(bytes == 1 && port == acpi_smi_cmd);
> > +
> > +    if ( dir == IOREQ_READ )
> > +    {
> > +        *val = inb(port);
> > +        return X86EMUL_OKAY;
> > +    }
> > +
> > +    outb(*val, port);
> > +    if ( *val == acpi_enable_value )
> > +       on_each_cpu(amd_disable_c1e, NULL, 1);
> > +
> > +    return X86EMUL_OKAY;
> > +}
> 
> I think you could do with even less logic duplication if instead you
> called check_disable_c1e() (then given an amd_ prefix of course)
> here. I'd then further consider to do away with
> pv_post_outb_hook in a follow-up patch, getting the PV side
> closer to the model used here.

Using check_disable_c1e incurs in one extra check that's not needed
here, since the port will always be acpi_smi_cmd in acpi_c1e_quirk.
In any case I agree it's better to use check_disable_c1e so there's
slightly less code repetition.

> 
> Not being the maintainer of this code, I'd nevertheless like to also
> suggest to use if/else here, making the function get away with
> just a single return point.

Ack, will resend with the fixes.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel