Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade resource

[Laszlo Ersek]

On 05/24/16 17:29, Ni, Ruiyu wrote:
> Laszlo,
> I am terribly sorry about that! For this patch serials, I didn't noticed your 
> r-b. That's why I offline asked Jeff to send his r-b. I can add your tested 
> by tag, there is no reason to ignore your review by!
> 
> For the dependency patch, I forgot your review by mail after a weekend.
> 
> I will search related mails when committing patches to avoid ignoring mails 
> again.(Outlook doesn't organize these mails from this mail list in thread 
> style)

Ah, sorry. In this case I'll clearly blame Outlook. It is plain
impossible to work with patches and review feedback if the MUA doesn't
support threading.

Barring a MUA switch on your side, I guess I could recommend reviewing
the patch series thread on GMANE (in the mailing list archive) one last
time, before committing the patches. GMANE has a great threading WebUI,
which would likely provide you with all the feedback for a patch series,
in a nice centralized view.

Thanks,
Laszlo

>> 在 2016年5月24日，下午8:28，Laszlo Ersek  写道：
>>
>> Ray,
>>
>>> On 05/18/16 13:35, Laszlo Ersek wrote:
>>>
>>> For #1 through #3:
>>> Reviewed-by: Laszlo Ersek 
>>
>> [snip]
>>
>>> - Anyway I'll leave the above two points up to your consideration. For 
>>> patch #4 too:
>>> Reviewed-by: Laszlo Ersek 
>>
>> I would *greatly* appreciate if you didn't ignore my (positive) reviews.
>>
>> Just because I'm not an official package maintainer for MdeModulePkg, I
>> do expect that my Reviewed-by tags be picked up, when I make the effort
>> to review MdeModulePkg patches.
>>
>> I have just noticed that you didn't pick up my above Reviewed-by tags,
>> for commits
>>
>> cf81d5a68052 MdeModulePkg/PciBus: use better name for local variables.
>> 48495aae386a MdeModulePkg/PciBus: Remove unused fields in PCI_BAR
>> ea669c1ba331 MdeModulePkg/PciBus: Use shorter global variable name
>> 05070c1b471b MdeModulePkg/PciBus: do not improperly degrade resource
>>
>> I see that you picked up my Tested-by tag (for all of the patches in
>> this series), but I didn't just test these patches, I also reviewed them.
>>
>> I see the exact same occur in commit
>>
>> 0b58c4894dad MdeModulePkg/PciHostBridgeDxe: Add CpuArch protocol
>> dependency
>>
>> I reviewed that patch on the list, but the committed version does not
>> have my R-b.
>>
>> In general, if you receive *any* kind of feedback tag from anyone in the
>> community, it is more or less your "duty" to preserve those tags when
>> you commit the patch. If you squander reviews that you get (even if they
>> are positive reviews), that's a big dis-incentive for future feedback.
>>
>> Thanks
>> Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade resource

[Ni, Ruiyu]

Laszlo,
I am terribly sorry about that! For this patch serials, I didn't noticed your 
r-b. That's why I offline asked Jeff to send his r-b. I can add your tested by 
tag, there is no reason to ignore your review by!

For the dependency patch, I forgot your review by mail after a weekend.

I will search related mails when committing patches to avoid ignoring mails 
again.(Outlook doesn't organize these mails from this mail list in thread style)

Thanks,
Ray

> 在 2016年5月24日，下午8:28，Laszlo Ersek  写道：
> 
> Ray,
> 
>> On 05/18/16 13:35, Laszlo Ersek wrote:
>> 
>> For #1 through #3:
>> Reviewed-by: Laszlo Ersek 
> 
> [snip]
> 
>> - Anyway I'll leave the above two points up to your consideration. For patch 
>> #4 too:
>> Reviewed-by: Laszlo Ersek 
> 
> I would *greatly* appreciate if you didn't ignore my (positive) reviews.
> 
> Just because I'm not an official package maintainer for MdeModulePkg, I
> do expect that my Reviewed-by tags be picked up, when I make the effort
> to review MdeModulePkg patches.
> 
> I have just noticed that you didn't pick up my above Reviewed-by tags,
> for commits
> 
> cf81d5a68052 MdeModulePkg/PciBus: use better name for local variables.
> 48495aae386a MdeModulePkg/PciBus: Remove unused fields in PCI_BAR
> ea669c1ba331 MdeModulePkg/PciBus: Use shorter global variable name
> 05070c1b471b MdeModulePkg/PciBus: do not improperly degrade resource
> 
> I see that you picked up my Tested-by tag (for all of the patches in
> this series), but I didn't just test these patches, I also reviewed them.
> 
> I see the exact same occur in commit
> 
> 0b58c4894dad MdeModulePkg/PciHostBridgeDxe: Add CpuArch protocol
> dependency
> 
> I reviewed that patch on the list, but the committed version does not
> have my R-b.
> 
> In general, if you receive *any* kind of feedback tag from anyone in the
> community, it is more or less your "duty" to preserve those tags when
> you commit the patch. If you squander reviews that you get (even if they
> are positive reviews), that's a big dis-incentive for future feedback.
> 
> Thanks
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade resource

[Laszlo Ersek]

Ray,

On 05/18/16 13:35, Laszlo Ersek wrote:

> For #1 through #3:
> Reviewed-by: Laszlo Ersek 

[snip]

> - Anyway I'll leave the above two points up to your consideration. For patch 
> #4 too:
> Reviewed-by: Laszlo Ersek 

I would *greatly* appreciate if you didn't ignore my (positive) reviews.

Just because I'm not an official package maintainer for MdeModulePkg, I
do expect that my Reviewed-by tags be picked up, when I make the effort
to review MdeModulePkg patches.

I have just noticed that you didn't pick up my above Reviewed-by tags,
for commits

cf81d5a68052 MdeModulePkg/PciBus: use better name for local variables.
48495aae386a MdeModulePkg/PciBus: Remove unused fields in PCI_BAR
ea669c1ba331 MdeModulePkg/PciBus: Use shorter global variable name
05070c1b471b MdeModulePkg/PciBus: do not improperly degrade resource

I see that you picked up my Tested-by tag (for all of the patches in
this series), but I didn't just test these patches, I also reviewed them.

I see the exact same occur in commit

0b58c4894dad MdeModulePkg/PciHostBridgeDxe: Add CpuArch protocol
 dependency

I reviewed that patch on the list, but the committed version does not
have my R-b.

In general, if you receive *any* kind of feedback tag from anyone in the
community, it is more or less your "duty" to preserve those tags when
you commit the patch. If you squander reviews that you get (even if they
are positive reviews), that's a big dis-incentive for future feedback.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade resource

[Fan, Jeff]

Reviewed-by: Jeff Fan <jeff@intel.com>

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Tuesday, May 17, 2016 10:04 AM
To: edk2-devel@lists.01.org
Cc: Ni, Ruiyu
Subject: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade 
resource

The patch serials refined the PciBus code and adds a new feature following PI 
spec 1.4a to not improperly degrade resource.

Ruiyu Ni (4):
  MdeModulePkg/PciBus: use better name for local variables.
  MdeModulePkg/PciBus: Remove unused fields in PCI_BAR
  MdeModulePkg/PciBus: Use shorter global variable name
  MdeModulePkg/PciBus: do not improperly degrade resource

 MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c|  6 +--
 MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h|  5 +-
 .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   | 58 +-
 .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 26 ++
 4 files changed, 65 insertions(+), 30 deletions(-)

--
2.7.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade resource

[Laszlo Ersek]

On 05/18/16 17:08, Ni, Ruiyu wrote:
> I agree to put ecr number in commit message.

Series
Tested-by: Laszlo Ersek <ler...@redhat.com>

I'll soon submit a patch that enables this new functionality for OvmfPkg.

Thank you!
Laszlo

>> 在 2016年5月18日，下午7:35，Laszlo Ersek <ler...@redhat.com> 写道：
>>
>> On 05/18/16 10:16, Ni, Ruiyu wrote:
>>>> -Original Message-
>>>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>>>> Sent: Tuesday, May 17, 2016 8:23 PM
>>>> To: Ni, Ruiyu <ruiyu...@intel.com>
>>>> Cc: edk2-de...@ml01.01.org
>>>> Subject: Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly 
>>>> degrade resource
>>>>
>>>>> On 05/17/16 04:03, Ruiyu Ni wrote:
>>>>> The patch serials refined the PciBus code and adds a new feature following
>>>>> PI spec 1.4a to not improperly degrade resource.
>>>>>
>>>>> Ruiyu Ni (4):
>>>>>  MdeModulePkg/PciBus: use better name for local variables.
>>>>>  MdeModulePkg/PciBus: Remove unused fields in PCI_BAR
>>>>>  MdeModulePkg/PciBus: Use shorter global variable name
>>>>>  MdeModulePkg/PciBus: do not improperly degrade resource
>>>>>
>>>>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c|  6 +--
>>>>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h|  5 +-
>>>>> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   | 58 
>>>>> +-
>>>>> .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 26 ++
>>>>> 4 files changed, 65 insertions(+), 30 deletions(-)
>>>>
>>>> * Please add the following reference to patch #4:
>>>>
>>>> https://mantis.uefi.org/mantis/view.php?id=1529
>>>
>>> I think it might not be proper to include the member only
>>> web page in the commit message.
>>
>> I disagree -- I think the mantis ticket number should always be included in 
>> a commit message, if the patch is related to a Mantis ticket. Sharing just 
>> the number of the mantis ticket does not disclose any information that is 
>> not also publicly available from the UEFI spec itself (the UEFI spec starts 
>> with a long changelog of Mantis tickets).
>>
>> The contents of the mantis ticket *are* private, but for actually reading a 
>> ticket, general UEFI membership and the appropriate WG membership are 
>> required too. So, capturing the mantis ticket number in the commit message 
>> helps contributors that have access to Mantis, without disclosing sensitive 
>> information to those who haven't.
>>
>> Personally I frequently go to Mantis to understand the background of UEFI or 
>> PI spec changes better. The ECRs attached to Mantis tickets often describe 
>> use cases that are not carried over to the spec, but are important 
>> background.
>>
>> In this specific case, the PI spec 1.4a, Volume 5, has the following in its 
>> changelog:
>>
>>  1529 address space granularity errata
>>
>>
>>>> * I'm interested in this work primarily due to GPU assignment to
>>>> QEMU/KVM virtual machines. So I built OVMF applied with these patches,
>>>> and checked if they made a difference for the PCI resource map, with an
>>>> Nvidia GTX750 assigned to the guest.
>>>>
>>>> There's no difference; the 256MB BAR is still allocated under 4GB.
>>>>
>>>> * Apparently, EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL must be a
>>>> singleton protocol; it should come from the platform firmware, not from
>>>> the option ROM. In that case, how could OVMF implement this protocol, so
>>>> that such a GPU BAR would be allocated high?
>>>
>>> Option ROM depends on the PCI resource assignment so it’s like a
>>> chicken-egg problem if we let option rom produce such protocol.
>>> Singleton is enough I think.
>>
>> I agree.
>>
>>>> Looking at the CheckDevice() spec, I guess we could hard-code some PCI
>>>> vendor and device IDs, and set AddrSpaceGranularity to 64. However,
>>>> that's the only thing we should do; I don't think we should do the rest
>>>> of the PCI BAR probing.
>>>
>>> PCI BAR probing is still needed. We do not change the granularity
>>> for 32bit BAR.
>>>>
>>>> ... The only thing I'd like to say to the resource allocator is,
>>>>
>>>> If CSM is disabled, then please don't degrade the MMIO64 BA

Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade resource

[Ni, Ruiyu]

I agree to put ecr number in commit message.

Thanks,
Ray

> 在 2016年5月18日，下午7:35，Laszlo Ersek <ler...@redhat.com> 写道：
> 
> On 05/18/16 10:16, Ni, Ruiyu wrote:
>>> -Original Message-
>>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>>> Sent: Tuesday, May 17, 2016 8:23 PM
>>> To: Ni, Ruiyu <ruiyu...@intel.com>
>>> Cc: edk2-de...@ml01.01.org
>>> Subject: Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly 
>>> degrade resource
>>> 
>>>> On 05/17/16 04:03, Ruiyu Ni wrote:
>>>> The patch serials refined the PciBus code and adds a new feature following
>>>> PI spec 1.4a to not improperly degrade resource.
>>>> 
>>>> Ruiyu Ni (4):
>>>>  MdeModulePkg/PciBus: use better name for local variables.
>>>>  MdeModulePkg/PciBus: Remove unused fields in PCI_BAR
>>>>  MdeModulePkg/PciBus: Use shorter global variable name
>>>>  MdeModulePkg/PciBus: do not improperly degrade resource
>>>> 
>>>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c|  6 +--
>>>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h|  5 +-
>>>> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   | 58 
>>>> +-
>>>> .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 26 ++
>>>> 4 files changed, 65 insertions(+), 30 deletions(-)
>>> 
>>> * Please add the following reference to patch #4:
>>> 
>>> https://mantis.uefi.org/mantis/view.php?id=1529
>> 
>> I think it might not be proper to include the member only
>> web page in the commit message.
> 
> I disagree -- I think the mantis ticket number should always be included in a 
> commit message, if the patch is related to a Mantis ticket. Sharing just the 
> number of the mantis ticket does not disclose any information that is not 
> also publicly available from the UEFI spec itself (the UEFI spec starts with 
> a long changelog of Mantis tickets).
> 
> The contents of the mantis ticket *are* private, but for actually reading a 
> ticket, general UEFI membership and the appropriate WG membership are 
> required too. So, capturing the mantis ticket number in the commit message 
> helps contributors that have access to Mantis, without disclosing sensitive 
> information to those who haven't.
> 
> Personally I frequently go to Mantis to understand the background of UEFI or 
> PI spec changes better. The ECRs attached to Mantis tickets often describe 
> use cases that are not carried over to the spec, but are important background.
> 
> In this specific case, the PI spec 1.4a, Volume 5, has the following in its 
> changelog:
> 
>  1529 address space granularity errata
> 
> 
>>> * I'm interested in this work primarily due to GPU assignment to
>>> QEMU/KVM virtual machines. So I built OVMF applied with these patches,
>>> and checked if they made a difference for the PCI resource map, with an
>>> Nvidia GTX750 assigned to the guest.
>>> 
>>> There's no difference; the 256MB BAR is still allocated under 4GB.
>>> 
>>> * Apparently, EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL must be a
>>> singleton protocol; it should come from the platform firmware, not from
>>> the option ROM. In that case, how could OVMF implement this protocol, so
>>> that such a GPU BAR would be allocated high?
>> 
>> Option ROM depends on the PCI resource assignment so it’s like a
>> chicken-egg problem if we let option rom produce such protocol.
>> Singleton is enough I think.
> 
> I agree.
> 
>>> Looking at the CheckDevice() spec, I guess we could hard-code some PCI
>>> vendor and device IDs, and set AddrSpaceGranularity to 64. However,
>>> that's the only thing we should do; I don't think we should do the rest
>>> of the PCI BAR probing.
>> 
>> PCI BAR probing is still needed. We do not change the granularity
>> for 32bit BAR.
>>> 
>>> ... The only thing I'd like to say to the resource allocator is,
>>> 
>>> If CSM is disabled, then please don't degrade the MMIO64 BARs of this
>>> device, just because it has an oprom.
>>> 
>>> (In fact, the device is not an incompatible PCI device.)
>> 
>> I agree. But another rule is MdeModulePkg cannot depend
>> on IntelFrameworkPkg. PciBusDxe is in MdeModulePkg; LegacyBios
>> protocol is defined in IntelFrameworkPkg.
> 
> That's fine; I'm not suggesting that PciBusDxe itself look for the legacy 
> BIOS protocol. Instead, platform code in OvmfPkg could look 

Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade resource

[Laszlo Ersek]

On 05/18/16 10:16, Ni, Ruiyu wrote:
>>-Original Message-
>>From: Laszlo Ersek [mailto:ler...@redhat.com]
>>Sent: Tuesday, May 17, 2016 8:23 PM
>>To: Ni, Ruiyu <ruiyu...@intel.com>
>>Cc: edk2-de...@ml01.01.org
>>Subject: Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade 
>>resource
>>
>>On 05/17/16 04:03, Ruiyu Ni wrote:
>>> The patch serials refined the PciBus code and adds a new feature following
>>> PI spec 1.4a to not improperly degrade resource.
>>>
>>> Ruiyu Ni (4):
>>>   MdeModulePkg/PciBus: use better name for local variables.
>>>   MdeModulePkg/PciBus: Remove unused fields in PCI_BAR
>>>   MdeModulePkg/PciBus: Use shorter global variable name
>>>   MdeModulePkg/PciBus: do not improperly degrade resource
>>>
>>>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c|  6 +--
>>>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h|  5 +-
>>>  .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   | 58 
>>> +-
>>>  .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 26 ++
>>>  4 files changed, 65 insertions(+), 30 deletions(-)
>>>
>>
>>* Please add the following reference to patch #4:
>>
>>https://mantis.uefi.org/mantis/view.php?id=1529
> 
> I think it might not be proper to include the member only
> web page in the commit message.

I disagree -- I think the mantis ticket number should always be included in a 
commit message, if the patch is related to a Mantis ticket. Sharing just the 
number of the mantis ticket does not disclose any information that is not also 
publicly available from the UEFI spec itself (the UEFI spec starts with a long 
changelog of Mantis tickets).

The contents of the mantis ticket *are* private, but for actually reading a 
ticket, general UEFI membership and the appropriate WG membership are required 
too. So, capturing the mantis ticket number in the commit message helps 
contributors that have access to Mantis, without disclosing sensitive 
information to those who haven't.

Personally I frequently go to Mantis to understand the background of UEFI or PI 
spec changes better. The ECRs attached to Mantis tickets often describe use 
cases that are not carried over to the spec, but are important background.

In this specific case, the PI spec 1.4a, Volume 5, has the following in its 
changelog:

  1529 address space granularity errata


>>* I'm interested in this work primarily due to GPU assignment to
>>QEMU/KVM virtual machines. So I built OVMF applied with these patches,
>>and checked if they made a difference for the PCI resource map, with an
>>Nvidia GTX750 assigned to the guest.
>>
>>There's no difference; the 256MB BAR is still allocated under 4GB.
>>
>>* Apparently, EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL must be a
>>singleton protocol; it should come from the platform firmware, not from
>>the option ROM. In that case, how could OVMF implement this protocol, so
>>that such a GPU BAR would be allocated high?
> 
> Option ROM depends on the PCI resource assignment so it’s like a
> chicken-egg problem if we let option rom produce such protocol.
> Singleton is enough I think.

I agree.

>>Looking at the CheckDevice() spec, I guess we could hard-code some PCI
>>vendor and device IDs, and set AddrSpaceGranularity to 64. However,
>>that's the only thing we should do; I don't think we should do the rest
>>of the PCI BAR probing.
> 
> PCI BAR probing is still needed. We do not change the granularity
> for 32bit BAR.
>>
>>... The only thing I'd like to say to the resource allocator is,
>>
>>  If CSM is disabled, then please don't degrade the MMIO64 BARs of this
>>  device, just because it has an oprom.
>>
>>(In fact, the device is not an incompatible PCI device.)
> 
> I agree. But another rule is MdeModulePkg cannot depend
> on IntelFrameworkPkg. PciBusDxe is in MdeModulePkg; LegacyBios
> protocol is defined in IntelFrameworkPkg.

That's fine; I'm not suggesting that PciBusDxe itself look for the legacy BIOS 
protocol. Instead, platform code in OvmfPkg could look for the legacy BIOS 
protocol (or determine CSM presence in another way), and then instruct 
PciBusDxe through a well-defined interface not to degrade the 64-bit MMIO BARs.

My interest is in such a well-defined interface.

>>* By following the links in Mantis #1529 (recursively), I arrived at a
>>message that points to the following driver:
>>
>>  MdeModulePkg/Bus/Pci/IncompatiblePciDeviceSupportDxe
>>
>>OVMF does not include this driver at the moment. It is a small driver
>>and I guess we could fork it for Ovmf

Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade resource

[Ni, Ruiyu]

64F5B107-317A-4857-9D71-5C4165493076


Regards,
Ray

>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Tuesday, May 17, 2016 8:23 PM
>To: Ni, Ruiyu <ruiyu...@intel.com>
>Cc: edk2-de...@ml01.01.org
>Subject: Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade 
>resource
>
>On 05/17/16 04:03, Ruiyu Ni wrote:
>> The patch serials refined the PciBus code and adds a new feature following
>> PI spec 1.4a to not improperly degrade resource.
>>
>> Ruiyu Ni (4):
>>   MdeModulePkg/PciBus: use better name for local variables.
>>   MdeModulePkg/PciBus: Remove unused fields in PCI_BAR
>>   MdeModulePkg/PciBus: Use shorter global variable name
>>   MdeModulePkg/PciBus: do not improperly degrade resource
>>
>>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c|  6 +--
>>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h|  5 +-
>>  .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   | 58 
>> +-
>>  .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 26 ++
>>  4 files changed, 65 insertions(+), 30 deletions(-)
>>
>
>* Please add the following reference to patch #4:
>
>https://mantis.uefi.org/mantis/view.php?id=1529

I think it might not be proper to include the member only
web page in the commit message.

>
>* I'm interested in this work primarily due to GPU assignment to
>QEMU/KVM virtual machines. So I built OVMF applied with these patches,
>and checked if they made a difference for the PCI resource map, with an
>Nvidia GTX750 assigned to the guest.
>
>There's no difference; the 256MB BAR is still allocated under 4GB.
>
>* Apparently, EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL must be a
>singleton protocol; it should come from the platform firmware, not from
>the option ROM. In that case, how could OVMF implement this protocol, so
>that such a GPU BAR would be allocated high?

Option ROM depends on the PCI resource assignment so it's like a
chicken-egg problem if we let option rom produce such protocol.
Singleton is enough I think.

>
>Looking at the CheckDevice() spec, I guess we could hard-code some PCI
>vendor and device IDs, and set AddrSpaceGranularity to 64. However,
>that's the only thing we should do; I don't think we should do the rest
>of the PCI BAR probing.

PCI BAR probing is still needed. We do not change the granularity
for 32bit BAR.

>
>... The only thing I'd like to say to the resource allocator is,
>
>  If CSM is disabled, then please don't degrade the MMIO64 BARs of this
>  device, just because it has an oprom.
>
>(In fact, the device is not an incompatible PCI device.)

I agree. But another rule is MdeModulePkg cannot depend
on IntelFrameworkPkg. PciBusDxe is in MdeModulePkg; LegacyBios
protocol is defined in IntelFrameworkPkg.

>
>* By following the links in Mantis #1529 (recursively), I arrived at a
>message that points to the following driver:
>
>  MdeModulePkg/Bus/Pci/IncompatiblePciDeviceSupportDxe
>
>OVMF does not include this driver at the moment. It is a small driver
>and I guess we could fork it for OvmfPkg, but how should we fill in the
>"mIncompatiblePciDeviceList" array?
>
Try to use below value to disable all MMIO degrade.
Not tested.

GLOBAL_REMOVE_IF_UNREFERENCED UINT64 mIncompatiblePciDeviceList[] = {
  //
  // DEVICE_INF_TAG,
  // PCI_DEVICE_ID (VendorID, DeviceID, Revision, SubVendorId, SubDeviceId),
  // DEVICE_RES_TAG,
  // ResType,  GFlag , SFlag,   Granularity,  RangeMin,
  // RangeMax, Offset, AddrLen
  //
  DEVICE_INF_TAG,
  PCI_DEVICE_ID(DEVICE_ID_NOCARE, DEVICE_ID_NOCARE, DEVICE_ID_NOCARE, 
DEVICE_ID_NOCARE, DEVICE_ID_NOCARE),
  DEVICE_RES_TAG,
  PCI_BAR_TYPE_MEM,
  PCI_ACPI_UNUSED,
  PCI_ACPI_UNUSED,
 64,
  PCI_ACPI_UNUSED,
  PCI_BAR_OLD_ALIGN,
  PCI_BAR_ALL,
  PCI_BAR_NOCHANGE,


>Thanks
>Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade resource

[Laszlo Ersek]

On 05/17/16 04:03, Ruiyu Ni wrote:
> The patch serials refined the PciBus code and adds a new feature following
> PI spec 1.4a to not improperly degrade resource.
> 
> Ruiyu Ni (4):
>   MdeModulePkg/PciBus: use better name for local variables.
>   MdeModulePkg/PciBus: Remove unused fields in PCI_BAR
>   MdeModulePkg/PciBus: Use shorter global variable name
>   MdeModulePkg/PciBus: do not improperly degrade resource
> 
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c|  6 +--
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h|  5 +-
>  .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   | 58 
> +-
>  .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 26 ++
>  4 files changed, 65 insertions(+), 30 deletions(-)
> 

* Please add the following reference to patch #4:

https://mantis.uefi.org/mantis/view.php?id=1529

* I'm interested in this work primarily due to GPU assignment to
QEMU/KVM virtual machines. So I built OVMF applied with these patches,
and checked if they made a difference for the PCI resource map, with an
Nvidia GTX750 assigned to the guest.

There's no difference; the 256MB BAR is still allocated under 4GB.

* Apparently, EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL must be a
singleton protocol; it should come from the platform firmware, not from
the option ROM. In that case, how could OVMF implement this protocol, so
that such a GPU BAR would be allocated high?

Looking at the CheckDevice() spec, I guess we could hard-code some PCI
vendor and device IDs, and set AddrSpaceGranularity to 64. However,
that's the only thing we should do; I don't think we should do the rest
of the PCI BAR probing.

... The only thing I'd like to say to the resource allocator is,

  If CSM is disabled, then please don't degrade the MMIO64 BARs of this
  device, just because it has an oprom.

(In fact, the device is not an incompatible PCI device.)

* By following the links in Mantis #1529 (recursively), I arrived at a
message that points to the following driver:

  MdeModulePkg/Bus/Pci/IncompatiblePciDeviceSupportDxe

OVMF does not include this driver at the moment. It is a small driver
and I guess we could fork it for OvmfPkg, but how should we fill in the
"mIncompatiblePciDeviceList" array?

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch 0/4] MdeModulePkg/PciBus Do not improperly degrade resource

[Ruiyu Ni]

The patch serials refined the PciBus code and adds a new feature following
PI spec 1.4a to not improperly degrade resource.

Ruiyu Ni (4):
  MdeModulePkg/PciBus: use better name for local variables.
  MdeModulePkg/PciBus: Remove unused fields in PCI_BAR
  MdeModulePkg/PciBus: Use shorter global variable name
  MdeModulePkg/PciBus: do not improperly degrade resource

 MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c|  6 +--
 MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h|  5 +-
 .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   | 58 +-
 .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 26 ++
 4 files changed, 65 insertions(+), 30 deletions(-)

-- 
2.7.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel