| Message ID | 1331308660-20787-1-git-send-email-mark.langsdorf@calxeda.com |
|---|---|
| State | New |
| Headers | show |
On 9 March 2012 15:57, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: > Since the ram_size field of arm_boot_info is only an int, don't set > that field to more than INT_MAX. Signed vs unsigned comparison > overruns are possible otherwise. Can't we just make arm_boot_info.ram_size a uint32_t (propagating through signedness fixes as required) ? Actually it should probably be a target_phys_addr_t, thinking ahead to adding LPAE support. -- PMM
On 03/09/2012 10:13 AM, Peter Maydell wrote: > On 9 March 2012 15:57, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: >> Since the ram_size field of arm_boot_info is only an int, don't set >> that field to more than INT_MAX. Signed vs unsigned comparison >> overruns are possible otherwise. > > Can't we just make arm_boot_info.ram_size a uint32_t (propagating through > signedness fixes as required) ? > > Actually it should probably be a target_phys_addr_t, thinking ahead > to adding LPAE support. It really should be a size_t, per the upthread discussion with Andreas Faerber. I'll take a stab at the patch, but it touches a lot of code that I don't really have a way to test so I'm a bit dubious. --Mark Langsdorf Calxeda, Inc.
On 09.03.2012, at 17:40, Mark Langsdorf wrote: > On 03/09/2012 10:13 AM, Peter Maydell wrote: >> On 9 March 2012 15:57, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: >>> Since the ram_size field of arm_boot_info is only an int, don't set >>> that field to more than INT_MAX. Signed vs unsigned comparison >>> overruns are possible otherwise. >> >> Can't we just make arm_boot_info.ram_size a uint32_t (propagating through >> signedness fixes as required) ? >> >> Actually it should probably be a target_phys_addr_t, thinking ahead >> to adding LPAE support. > > It really should be a size_t, per the upthread discussion with Andreas > Faerber. No, Andreas is wrong. Host data types have nothing to do in target ram fiddling code. The type you're searching for is "the size of physical address space the guest can handle" and that's target_phys_addr_t. Period. Alex
Am 09.03.2012 19:22, schrieb Alexander Graf: > > On 09.03.2012, at 17:40, Mark Langsdorf wrote: > >> On 03/09/2012 10:13 AM, Peter Maydell wrote: >>> On 9 March 2012 15:57, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: >>>> Since the ram_size field of arm_boot_info is only an int, don't set >>>> that field to more than INT_MAX. Signed vs unsigned comparison >>>> overruns are possible otherwise. >>> >>> Can't we just make arm_boot_info.ram_size a uint32_t (propagating through >>> signedness fixes as required) ? >>> >>> Actually it should probably be a target_phys_addr_t, thinking ahead >>> to adding LPAE support. >> >> It really should be a size_t, per the upthread discussion with Andreas >> Faerber. > > No, Andreas is wrong. Host data types have nothing to do in target ram fiddling code. The type you're searching for is "the size of physical address space the guest can handle" and that's target_phys_addr_t. Period. That is exactly where we disagree: It's not "target ram fiddling code". It's the host loading binaries from disk to host RAM. The Memory API constructs for wiring it up in guest RAM are outside the scope of this discussion. If we can't load a 4 GB file into our host RAM, there's no point bloating our APIs with unreadable types as arguments such as "target_phys_addr_t max_size" just because a 64-bit guest on a 32-bit host has a larger virtual address space. It's not an address, so if we end up needing a special type it should certainly not be any ...addr_t type. Typedef target_phys_size_t if you see a need but keep our APIs clean please. Alex, you say you don't care about infrastructure, well this IS infrastructure and I care about its API and usability. :) Andreas
On 09.03.2012, at 20:03, Andreas Färber wrote: > Am 09.03.2012 19:22, schrieb Alexander Graf: >> >> On 09.03.2012, at 17:40, Mark Langsdorf wrote: >> >>> On 03/09/2012 10:13 AM, Peter Maydell wrote: >>>> On 9 March 2012 15:57, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: >>>>> Since the ram_size field of arm_boot_info is only an int, don't set >>>>> that field to more than INT_MAX. Signed vs unsigned comparison >>>>> overruns are possible otherwise. >>>> >>>> Can't we just make arm_boot_info.ram_size a uint32_t (propagating through >>>> signedness fixes as required) ? >>>> >>>> Actually it should probably be a target_phys_addr_t, thinking ahead >>>> to adding LPAE support. >>> >>> It really should be a size_t, per the upthread discussion with Andreas >>> Faerber. >> >> No, Andreas is wrong. Host data types have nothing to do in target ram fiddling code. The type you're searching for is "the size of physical address space the guest can handle" and that's target_phys_addr_t. Period. > > That is exactly where we disagree: It's not "target ram fiddling code". > > It's the host loading binaries from disk to host RAM. No, it's an offset into guest RAM. Right now the interface is using target_phys_addr_t addr int size what if instead if would be using target_phys_addr_t start target_phys_addr_t end It would still be the same thing semantically. We have a start address and an end address that we can put data in. The fact that the image is backed by some POSIX layers in the background is a minor detail. It could just as well go through the block layer and be read via libcurl from http. The semantic of size is addr + size = end Alex
diff --git a/hw/highbank.c b/hw/highbank.c index 489c00e..577284a 100644 --- a/hw/highbank.c +++ b/hw/highbank.c @@ -306,7 +306,7 @@ static void highbank_init(ram_addr_t ram_size, sysbus_connect_irq(sysbus_from_qdev(dev), 2, pic[82]); } - highbank_binfo.ram_size = ram_size; + highbank_binfo.ram_size = (ram_size < INT_MAX ? ram_size : INT_MAX); highbank_binfo.kernel_filename = kernel_filename; highbank_binfo.kernel_cmdline = kernel_cmdline; highbank_binfo.initrd_filename = initrd_filename;
Since the ram_size field of arm_boot_info is only an int, don't set that field to more than INT_MAX. Signed vs unsigned comparison overruns are possible otherwise. Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com> --- hw/highbank.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)