diff mbox

[v5,5/5] nvdimm acpi: add _CRS

Message ID 1456919441-101204-6-git-send-email-guangrong.xiao@linux.intel.com
State New, archived
Headers show

Commit Message

Xiao Guangrong March 2, 2016, 11:50 a.m. UTC
As Igor suggested that we can report the BIOS patched operation region
so that OSPM could see that particular range is in use and be able to
notice conflicts if it happens some day

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Michael S. Tsirkin March 3, 2016, 1:29 p.m. UTC | #1
On Wed, Mar 02, 2016 at 07:50:41PM +0800, Xiao Guangrong wrote:
> As Igor suggested that we can report the BIOS patched operation region
> so that OSPM could see that particular range is in use and be able to
> notice conflicts if it happens some day
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>

This is reserved RAM, exposing it in _CRS makes no sense to me.


> ---
>  hw/acpi/nvdimm.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 5a17ee2..43fd4c5 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -578,6 +578,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>                                GArray *table_data, GArray *linker)
>  {
>      Aml *ssdt, *sb_scope, *dev, *field;
> +    Aml *min_addr, *max_addr, *mr32, *method, *crs;
>      int mem_addr_offset, nvdimm_ssdt;
>  
>      acpi_add_table(table_offsets, table_data);
> @@ -602,6 +603,32 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>       */
>      aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
>  
> +    /*
> +     * report the dsm memory so that OSPM could see that particular range is
> +     * in use and be able to notice conflicts if it happens some day.
> +     */
> +    method = aml_method("_CRS", 0, AML_SERIALIZED);
> +    crs = aml_resource_template();
> +    aml_append(crs, aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> +                                     AML_MAX_FIXED, AML_CACHEABLE,
> +                                     AML_READ_WRITE,
> +                                     0, 0x0, 0xFFFFFFFE, 0,
> +                                     TARGET_PAGE_SIZE));
> +    aml_append(method, aml_name_decl("MR32", crs));
> +    mr32 = aml_name("MR32");
> +    aml_append(method, aml_create_dword_field(mr32, aml_int(10), "MIN"));
> +    aml_append(method, aml_create_dword_field(mr32, aml_int(14), "MAX"));
> +
> +    min_addr = aml_name("MIN");
> +    max_addr = aml_name("MAX");
> +
> +    aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), min_addr));
> +    aml_append(method, aml_add(min_addr, aml_int(TARGET_PAGE_SIZE),
> +                               max_addr));
> +    aml_append(method, aml_decrement(max_addr));
> +    aml_append(method, aml_return(mr32));
> +    aml_append(dev, method);
> +
>      /* map DSM memory and IO into ACPI namespace. */
>      aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO,
>                 aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
> -- 
> 1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiao Guangrong March 3, 2016, 2:05 p.m. UTC | #2
On 03/03/2016 09:29 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2016 at 07:50:41PM +0800, Xiao Guangrong wrote:
>> As Igor suggested that we can report the BIOS patched operation region
>> so that OSPM could see that particular range is in use and be able to
>> notice conflicts if it happens some day
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>
> This is reserved RAM, exposing it in _CRS makes no sense to me.

As more and more memory will be reserved by BIOS/QEMU, report the
information to OSPM and let it check the potential error is bad,
no? :)

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin March 3, 2016, 2:48 p.m. UTC | #3
On Thu, Mar 03, 2016 at 10:05:31PM +0800, Xiao Guangrong wrote:
> 
> 
> On 03/03/2016 09:29 PM, Michael S. Tsirkin wrote:
> >On Wed, Mar 02, 2016 at 07:50:41PM +0800, Xiao Guangrong wrote:
> >>As Igor suggested that we can report the BIOS patched operation region
> >>so that OSPM could see that particular range is in use and be able to
> >>notice conflicts if it happens some day
> >>
> >>Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >
> >This is reserved RAM, exposing it in _CRS makes no sense to me.
> 
> As more and more memory will be reserved by BIOS/QEMU, report the
> information to OSPM and let it check the potential error is bad,
> no? :)

guest has enough info to detect conflicts if it wishes to.
IIUC _CRS is not intended for RAM, it's for MMIO
resources, if it works for RAM that's an accident.
Igor Mammedov March 7, 2016, 12:16 p.m. UTC | #4
On Thu, 3 Mar 2016 16:48:55 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Mar 03, 2016 at 10:05:31PM +0800, Xiao Guangrong wrote:
> > 
> > 
> > On 03/03/2016 09:29 PM, Michael S. Tsirkin wrote:  
> > >On Wed, Mar 02, 2016 at 07:50:41PM +0800, Xiao Guangrong wrote:  
> > >>As Igor suggested that we can report the BIOS patched operation region
> > >>so that OSPM could see that particular range is in use and be able to
> > >>notice conflicts if it happens some day
> > >>
> > >>Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>  
> > >
> > >This is reserved RAM, exposing it in _CRS makes no sense to me.  
> > 
> > As more and more memory will be reserved by BIOS/QEMU, report the
> > information to OSPM and let it check the potential error is bad,
> > no? :)  
> 
> guest has enough info to detect conflicts if it wishes to.
> IIUC _CRS is not intended for RAM, it's for MMIO
> resources, if it works for RAM that's an accident.
If range isn't reserved here, then guest might assume that it's
free to use it for a PCI device since PCI0._CRS reports it
as available.
So we should either reserve range or punch a hole in PCI0._CRS.
Reserving ranges is simpler and that's what we've switched to
from manual hole punching, see PCI/CPU/Memory hotplug and other
motherboard resources.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin March 7, 2016, 12:22 p.m. UTC | #5
On Mon, Mar 07, 2016 at 01:16:48PM +0100, Igor Mammedov wrote:
> On Thu, 3 Mar 2016 16:48:55 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Mar 03, 2016 at 10:05:31PM +0800, Xiao Guangrong wrote:
> > > 
> > > 
> > > On 03/03/2016 09:29 PM, Michael S. Tsirkin wrote:  
> > > >On Wed, Mar 02, 2016 at 07:50:41PM +0800, Xiao Guangrong wrote:  
> > > >>As Igor suggested that we can report the BIOS patched operation region
> > > >>so that OSPM could see that particular range is in use and be able to
> > > >>notice conflicts if it happens some day
> > > >>
> > > >>Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>  
> > > >
> > > >This is reserved RAM, exposing it in _CRS makes no sense to me.  
> > > 
> > > As more and more memory will be reserved by BIOS/QEMU, report the
> > > information to OSPM and let it check the potential error is bad,
> > > no? :)  
> > 
> > guest has enough info to detect conflicts if it wishes to.
> > IIUC _CRS is not intended for RAM, it's for MMIO
> > resources, if it works for RAM that's an accident.
> If range isn't reserved here, then guest might assume that it's
> free to use it for a PCI device since PCI0._CRS reports it
> as available.

Does it really? I thought it's guest RAM allocated by BIOS, as opposed
to PCI memory. Am I wrong?

> So we should either reserve range or punch a hole in PCI0._CRS.
> Reserving ranges is simpler and that's what we've switched to
> from manual hole punching, see PCI/CPU/Memory hotplug and other
> motherboard resources.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Igor Mammedov March 7, 2016, 2:49 p.m. UTC | #6
On Mon, 7 Mar 2016 14:22:38 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Mar 07, 2016 at 01:16:48PM +0100, Igor Mammedov wrote:
> > On Thu, 3 Mar 2016 16:48:55 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Thu, Mar 03, 2016 at 10:05:31PM +0800, Xiao Guangrong wrote:  
> > > > 
> > > > 
> > > > On 03/03/2016 09:29 PM, Michael S. Tsirkin wrote:    
> > > > >On Wed, Mar 02, 2016 at 07:50:41PM +0800, Xiao Guangrong wrote:    
> > > > >>As Igor suggested that we can report the BIOS patched operation region
> > > > >>so that OSPM could see that particular range is in use and be able to
> > > > >>notice conflicts if it happens some day
> > > > >>
> > > > >>Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>    
> > > > >
> > > > >This is reserved RAM, exposing it in _CRS makes no sense to me.    
> > > > 
> > > > As more and more memory will be reserved by BIOS/QEMU, report the
> > > > information to OSPM and let it check the potential error is bad,
> > > > no? :)    
> > > 
> > > guest has enough info to detect conflicts if it wishes to.
> > > IIUC _CRS is not intended for RAM, it's for MMIO
> > > resources, if it works for RAM that's an accident.  
> > If range isn't reserved here, then guest might assume that it's
> > free to use it for a PCI device since PCI0._CRS reports it
> > as available.  
> 
> Does it really? I thought it's guest RAM allocated by BIOS, as opposed
> to PCI memory. Am I wrong?
Maybe I'm wrong,
but aren't RAM and PCI memory mapped into the same physical address space?
So what would happen when PCI MMIO BAR would be mapped over above range,
since guest thinks it's free to use it as unused resource?


> 
> > So we should either reserve range or punch a hole in PCI0._CRS.
> > Reserving ranges is simpler and that's what we've switched to
> > from manual hole punching, see PCI/CPU/Memory hotplug and other
> > motherboard resources.  

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin March 7, 2016, 3:09 p.m. UTC | #7
On Mon, Mar 07, 2016 at 03:49:47PM +0100, Igor Mammedov wrote:
> On Mon, 7 Mar 2016 14:22:38 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Mar 07, 2016 at 01:16:48PM +0100, Igor Mammedov wrote:
> > > On Thu, 3 Mar 2016 16:48:55 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Thu, Mar 03, 2016 at 10:05:31PM +0800, Xiao Guangrong wrote:  
> > > > > 
> > > > > 
> > > > > On 03/03/2016 09:29 PM, Michael S. Tsirkin wrote:    
> > > > > >On Wed, Mar 02, 2016 at 07:50:41PM +0800, Xiao Guangrong wrote:    
> > > > > >>As Igor suggested that we can report the BIOS patched operation region
> > > > > >>so that OSPM could see that particular range is in use and be able to
> > > > > >>notice conflicts if it happens some day
> > > > > >>
> > > > > >>Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>    
> > > > > >
> > > > > >This is reserved RAM, exposing it in _CRS makes no sense to me.    
> > > > > 
> > > > > As more and more memory will be reserved by BIOS/QEMU, report the
> > > > > information to OSPM and let it check the potential error is bad,
> > > > > no? :)    
> > > > 
> > > > guest has enough info to detect conflicts if it wishes to.
> > > > IIUC _CRS is not intended for RAM, it's for MMIO
> > > > resources, if it works for RAM that's an accident.  
> > > If range isn't reserved here, then guest might assume that it's
> > > free to use it for a PCI device since PCI0._CRS reports it
> > > as available.  
> > 
> > Does it really? I thought it's guest RAM allocated by BIOS, as opposed
> > to PCI memory. Am I wrong?
> Maybe I'm wrong,
> but aren't RAM and PCI memory mapped into the same physical address space?

They are in the same address space but IIRC MMIO has lower priority.

> So what would happen when PCI MMIO BAR would be mapped over above range,
> since guest thinks it's free to use it as unused resource?

IIRC, allocating MMIO BAR over RAM would make the MMIO invisible,
irrespective of whether the RAM range is being used for anything.

> 
> > 
> > > So we should either reserve range or punch a hole in PCI0._CRS.
> > > Reserving ranges is simpler and that's what we've switched to
> > > from manual hole punching, see PCI/CPU/Memory hotplug and other
> > > motherboard resources.  
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Igor Mammedov March 7, 2016, 4:17 p.m. UTC | #8
On Mon, 7 Mar 2016 17:09:32 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Mar 07, 2016 at 03:49:47PM +0100, Igor Mammedov wrote:
> > On Mon, 7 Mar 2016 14:22:38 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Mon, Mar 07, 2016 at 01:16:48PM +0100, Igor Mammedov wrote:  
> > > > On Thu, 3 Mar 2016 16:48:55 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >     
> > > > > On Thu, Mar 03, 2016 at 10:05:31PM +0800, Xiao Guangrong wrote:    
> > > > > > 
> > > > > > 
> > > > > > On 03/03/2016 09:29 PM, Michael S. Tsirkin wrote:      
> > > > > > >On Wed, Mar 02, 2016 at 07:50:41PM +0800, Xiao Guangrong wrote:      
> > > > > > >>As Igor suggested that we can report the BIOS patched operation region
> > > > > > >>so that OSPM could see that particular range is in use and be able to
> > > > > > >>notice conflicts if it happens some day
> > > > > > >>
> > > > > > >>Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>      
> > > > > > >
> > > > > > >This is reserved RAM, exposing it in _CRS makes no sense to me.      
> > > > > > 
> > > > > > As more and more memory will be reserved by BIOS/QEMU, report the
> > > > > > information to OSPM and let it check the potential error is bad,
> > > > > > no? :)      
> > > > > 
> > > > > guest has enough info to detect conflicts if it wishes to.
> > > > > IIUC _CRS is not intended for RAM, it's for MMIO
> > > > > resources, if it works for RAM that's an accident.    
> > > > If range isn't reserved here, then guest might assume that it's
> > > > free to use it for a PCI device since PCI0._CRS reports it
> > > > as available.    
> > > 
> > > Does it really? I thought it's guest RAM allocated by BIOS, as opposed
> > > to PCI memory. Am I wrong?  
> > Maybe I'm wrong,
> > but aren't RAM and PCI memory mapped into the same physical address space?  
> 
> They are in the same address space but IIRC MMIO has lower priority.
> 
> > So what would happen when PCI MMIO BAR would be mapped over above range,
> > since guest thinks it's free to use it as unused resource?  
> 
> IIRC, allocating MMIO BAR over RAM would make the MMIO invisible,
> irrespective of whether the RAM range is being used for anything.
An then driver would start writing 'garbage' to RAM, resulting in
strange guest behavior and not work PCI device.

that's why reserving region is a good idea if it could be done.

> 
> >   
> > >   
> > > > So we should either reserve range or punch a hole in PCI0._CRS.
> > > > Reserving ranges is simpler and that's what we've switched to
> > > > from manual hole punching, see PCI/CPU/Memory hotplug and other
> > > > motherboard resources.    

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin March 7, 2016, 4:19 p.m. UTC | #9
On Mon, Mar 07, 2016 at 05:17:19PM +0100, Igor Mammedov wrote:
> > > So what would happen when PCI MMIO BAR would be mapped over above range,
> > > since guest thinks it's free to use it as unused resource?  
> > 
> > IIRC, allocating MMIO BAR over RAM would make the MMIO invisible,
> > irrespective of whether the RAM range is being used for anything.
> An then driver would start writing 'garbage' to RAM, resulting in
> strange guest behavior and not work PCI device.

Do you observe such behaviour?

> that's why reserving region is a good idea if it could be done.

Which region? Reserve all of RAM in _CRS?

> > 
> > >   
> > > >   
> > > > > So we should either reserve range or punch a hole in PCI0._CRS.
> > > > > Reserving ranges is simpler and that's what we've switched to
> > > > > from manual hole punching, see PCI/CPU/Memory hotplug and other
> > > > > motherboard resources.    
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Igor Mammedov March 8, 2016, 9:06 a.m. UTC | #10
On Mon, 7 Mar 2016 18:19:46 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Mar 07, 2016 at 05:17:19PM +0100, Igor Mammedov wrote:
> > > > So what would happen when PCI MMIO BAR would be mapped over above range,
> > > > since guest thinks it's free to use it as unused resource?    
> > > 
> > > IIRC, allocating MMIO BAR over RAM would make the MMIO invisible,
> > > irrespective of whether the RAM range is being used for anything.  
> > An then driver would start writing 'garbage' to RAM, resulting in
> > strange guest behavior and not work PCI device.  
> 
> Do you observe such behaviour?
> 
> > that's why reserving region is a good idea if it could be done.  
> 
> Which region? Reserve all of RAM in _CRS?
Ops, I'm wrong here since PCI0._CRS doesn't include RAM, it's
not needed to reserve region there.

> 
> > >   
> > > >     
> > > > >     
> > > > > > So we should either reserve range or punch a hole in PCI0._CRS.
> > > > > > Reserving ranges is simpler and that's what we've switched to
> > > > > > from manual hole punching, see PCI/CPU/Memory hotplug and other
> > > > > > motherboard resources.      
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 5a17ee2..43fd4c5 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -578,6 +578,7 @@  static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                               GArray *table_data, GArray *linker)
 {
     Aml *ssdt, *sb_scope, *dev, *field;
+    Aml *min_addr, *max_addr, *mr32, *method, *crs;
     int mem_addr_offset, nvdimm_ssdt;
 
     acpi_add_table(table_offsets, table_data);
@@ -602,6 +603,32 @@  static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
      */
     aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
 
+    /*
+     * report the dsm memory so that OSPM could see that particular range is
+     * in use and be able to notice conflicts if it happens some day.
+     */
+    method = aml_method("_CRS", 0, AML_SERIALIZED);
+    crs = aml_resource_template();
+    aml_append(crs, aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
+                                     AML_MAX_FIXED, AML_CACHEABLE,
+                                     AML_READ_WRITE,
+                                     0, 0x0, 0xFFFFFFFE, 0,
+                                     TARGET_PAGE_SIZE));
+    aml_append(method, aml_name_decl("MR32", crs));
+    mr32 = aml_name("MR32");
+    aml_append(method, aml_create_dword_field(mr32, aml_int(10), "MIN"));
+    aml_append(method, aml_create_dword_field(mr32, aml_int(14), "MAX"));
+
+    min_addr = aml_name("MIN");
+    max_addr = aml_name("MAX");
+
+    aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), min_addr));
+    aml_append(method, aml_add(min_addr, aml_int(TARGET_PAGE_SIZE),
+                               max_addr));
+    aml_append(method, aml_decrement(max_addr));
+    aml_append(method, aml_return(mr32));
+    aml_append(dev, method);
+
     /* map DSM memory and IO into ACPI namespace. */
     aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO,
                aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));