Re: [Xen-devel] [PATCH RFC] VT-d: honor firmware-first mode in XSA-59 workaround code

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Jan Beulich wrote on 2014-05-23:
>>>> On 23.05.14 at 04:32, <yang.z.zhang@xxxxxxxxx> wrote:
>> Jan Beulich wrote on 2014-05-22:
>>> +static int __init hest_parse_cmc(const struct acpi_hest_header *hest_hdr,
>>> +                            void *data)
>>> +{
>>> +#ifdef CONFIG_X86_MCE
>> 
>> I didn't find where CONFIG_X86_MCE is defined. Do you have another
>> patch to define it?
> 
> No, and intentionally not (for the moment), because of ...
> 
>>> +   unsigned int i; +       const struct acpi_hest_ia_corrected *cmc; +     
>>> const
>>> struct acpi_hest_ia_error_bank *mc_bank; + +        if (hest_hdr->type !=
>>> ACPI_HEST_TYPE_IA32_CORRECTED_CHECK) +              return 0; + +   cmc =
>>> container_of(hest_hdr, const struct acpi_hest_ia_corrected, header);
>>> +   if (!cmc->enabled) +            return 0; + +   /* +     * We expect 
>>> HEST to
>>> provide a list of MC banks that report errors +      * in firmware first
>>> mode. Otherwise, return non-zero value to +  * indicate that we are
>>> done parsing HEST. +         */ +   if (!(cmc->flags &
>>> ACPI_HEST_FIRMWARE_FIRST) || !cmc->num_hardware_banks) +            return 
>>> 1; +
>>> +   printk(XENLOG_INFO HEST_PFX "Enabling Firmware First mode for
>>> +corrected errors.\n"); + + mc_bank = (const struct
>>> acpi_hest_ia_error_bank *)(cmc + 1); +      for (i = 0; i <
>>> cmc->num_hardware_banks; i++, mc_bank++)
>>> +           mce_disable_bank(mc_bank->bank_number);
>> 
>> Also no mce_diable_bak is founded.
> 
> ... this ...
> 
>>> +#else
>>> +# define acpi_disable_cmcff 1
>> 
>> You didn't define acpi_disable_cmcff if defined CONFIG_X86_MCE. It
>> will cause build error.
> 
> ... and this. I simply didn't want to discard stuff from the Linux
> file that we ought to be making use of eventually (I'd be hoping for
> the machine check maintainers to perhaps try to flesh this out
> subsequently). With the way it is now, things build and work. I'm
> adding a note to the commit message explaining this.
> 
>>> +static bool_t hest_match_type(const struct acpi_hest_header
>>> *hest_hdr, +                              const struct pci_dev *pdev)
>>> { +    unsigned int pos = pci_find_cap_offset(pdev->seg, pdev->bus, +
>>> PCI_SLOT(pdev->devfn), + PCI_FUNC(pdev->devfn), +                     
>>>                      PCI_CAP_ID_EXP); +    u8 pcie =
>>> (pci_conf_read16(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), +       
>>>                        PCI_FUNC(pdev->devfn), pos + PCI_EXP_FLAGS) & +
>>>               PCI_EXP_FLAGS_TYPE) / +              (PCI_EXP_FLAGS_TYPE
>>> & -PCI_EXP_FLAGS_TYPE);
>> 
>> I think right shift is much intuitively.
>> u8 pcie = (pci_conf_read16(pdev->seg,
>> pdev->bus,PCI_SLOT(pdev->devfn),PCI_FUNC(pdev->devfn), pos +
>> pdev->PCI_EXP_FLAGS) &
>> PCI_EXP_FLAGS_TYPE) >> 4;
> 
> But then who in the world is "4"? I sincerely dislike hardcoded
> numbers, and _intentionally_ replaced the 4 Linux uses here.
> 
>>> @@ -447,18 +448,26 @@ void pci_vtd_quirk(const struct pci_dev
>>>          }
>>>          
>>>          val = pci_conf_read32(seg, bus, dev, func, pos +
>>> PCI_ERR_UNCOR_MASK); -        pci_conf_write32(seg, bus, dev, func,
>>> pos + PCI_ERR_UNCOR_MASK, -                         val |
>>> PCI_ERR_UNC_UNSUP); -        val = pci_conf_read32(seg, bus, dev,
>>> func, pos + PCI_ERR_COR_MASK); -        pci_conf_write32(seg, bus,
>>> dev, func, pos + PCI_ERR_COR_MASK, -                         val |
>>> PCI_ERR_COR_ADV_NFAT); +        val2 = pci_conf_read32(seg, bus, dev,
>>> func, pos + PCI_ERR_COR_MASK); +        if ( (val & PCI_ERR_UNC_UNSUP)
>>> && (val2 & PCI_ERR_COR_ADV_NFAT) ) +            action = "Found
>>> masked";
>> 
>> What happened if dom0 unmasked it later?
> 
> That question you should have raised on the first of the XSA-59
> related patch sets, but the answer is we expect Dom0 to be well
> behaved. Of course, if you have a non-intrusive suggestion on how to enforce 
> this in Xen, I'm all ears.

So you mean we need to stare at Linux upstream to make sure it doesn't do any 
un-friendly modification to Xen.

> 
> Jan


Best regards,
Yang



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel