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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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 + 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.

Jan


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