| Message ID | 551B20D8.3010806@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
On 01/04/2015 00:34, Mark Cave-Ayland wrote: > On 30/03/15 12:47, Paolo Bonzini wrote: > >> On 30/03/2015 13:45, Peter Crosthwaite wrote: >>> Can the address_space_translate_address() length clamp be made >>> conditional on non-MMIO access as the RC fix? I submitted >>> c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 as I think its the right >>> thing to do regardless of memory type, but in reality it only fixes a >>> bug I encountered with RAM memory regions. The original code ignores >>> address_space_translate_internal() return-by-pointer length value >>> absolutely and the new code uses it absolutely. Should we just if the >>> whole thing, old vs new behaviour on MMIO vs non-MMIO? >>> >>> Happy to submit that fixup if that's the accepted plan. >> >> I submitted what I think is the right fix (sorry Mark for misspelling >> your email address). I think you're patch is correct, so I'd rather not >> introduce hacks for the release; we can either revert it, or fix ioport.c. > > Ah wait a second - the same patch c3c1bb99 also breaks reset on SPARC32: > > $ ./qemu-system-sparc -prom-env 'auto-boot?=false' -nographic > Configuration device id QEMU version 1 machine id 32 > Probing SBus slot 0 offset 0 > Probing SBus slot 1 offset 0 > Probing SBus slot 2 offset 0 > Probing SBus slot 3 offset 0 > Probing SBus slot 4 offset 0 > Probing SBus slot 5 offset 0 > Invalid FCode start byte > CPUs: 1 x FMI,MB86904 > UUID: 00000000-0000-0000-0000-000000000000 > Welcome to OpenBIOS v1.1 built on Mar 12 2015 08:08 > Type 'help' for detailed information > > 0 > reset-all Unhandled Exception 0x00000029 > PC = 0xffd10c08 NPC = 0xffd10c0c > Stopping execution > > The reset is controlled by writing a 1 to the hardware reset register at > physical address 0x71f00000 (system-control according to "info mtree") > which is handled in hw/misc/slavio_misc.c. > > Given that SYSCTRL_SIZE isn't used at all (and the access size for > slavio_led_mem_ops also looks wrong), the following seems to be the > correct fix for the reset and LED access: > > > --- a/hw/misc/slavio_misc.c > +++ b/hw/misc/slavio_misc.c > @@ -68,6 +68,7 @@ typedef struct APCState { > } APCState; > > #define MISC_SIZE 1 > +#define LED_SIZE 2 > #define SYSCTRL_SIZE 4 > > #define AUX1_TC 0x02 > @@ -452,13 +453,13 @@ static int slavio_misc_init1(SysBusDevice *sbd) > /* 16 bit registers */ > /* ss600mp diag LEDs */ > memory_region_init_io(&s->led_iomem, OBJECT(s), &slavio_led_mem_ops, s, > - "leds", MISC_SIZE); > + "leds", LED_SIZE); > sysbus_init_mmio(sbd, &s->led_iomem); > > /* 32 bit registers */ > /* System control */ > memory_region_init_io(&s->sysctrl_iomem, OBJECT(s), > &slavio_sysctrl_mem_ops, s, > - "system-control", MISC_SIZE); > + "system-control", SYSCTRL_SIZE); > sysbus_init_mmio(sbd, &s->sysctrl_iomem); > > /* AUX 1 (Misc System Functions) */ > > > Then again it is getting quite late in the release cycle... Yes, it's better to revert for now. Can you submit the patch above, so I include it in 2.4 and keep bisectability? Paolo
On 01/04/15 08:55, Paolo Bonzini wrote: > On 01/04/2015 00:34, Mark Cave-Ayland wrote: >> On 30/03/15 12:47, Paolo Bonzini wrote: >> >>> On 30/03/2015 13:45, Peter Crosthwaite wrote: >>>> Can the address_space_translate_address() length clamp be made >>>> conditional on non-MMIO access as the RC fix? I submitted >>>> c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 as I think its the right >>>> thing to do regardless of memory type, but in reality it only fixes a >>>> bug I encountered with RAM memory regions. The original code ignores >>>> address_space_translate_internal() return-by-pointer length value >>>> absolutely and the new code uses it absolutely. Should we just if the >>>> whole thing, old vs new behaviour on MMIO vs non-MMIO? >>>> >>>> Happy to submit that fixup if that's the accepted plan. >>> >>> I submitted what I think is the right fix (sorry Mark for misspelling >>> your email address). I think you're patch is correct, so I'd rather not >>> introduce hacks for the release; we can either revert it, or fix ioport.c. >> >> Ah wait a second - the same patch c3c1bb99 also breaks reset on SPARC32: >> >> $ ./qemu-system-sparc -prom-env 'auto-boot?=false' -nographic >> Configuration device id QEMU version 1 machine id 32 >> Probing SBus slot 0 offset 0 >> Probing SBus slot 1 offset 0 >> Probing SBus slot 2 offset 0 >> Probing SBus slot 3 offset 0 >> Probing SBus slot 4 offset 0 >> Probing SBus slot 5 offset 0 >> Invalid FCode start byte >> CPUs: 1 x FMI,MB86904 >> UUID: 00000000-0000-0000-0000-000000000000 >> Welcome to OpenBIOS v1.1 built on Mar 12 2015 08:08 >> Type 'help' for detailed information >> >> 0 > reset-all Unhandled Exception 0x00000029 >> PC = 0xffd10c08 NPC = 0xffd10c0c >> Stopping execution >> >> The reset is controlled by writing a 1 to the hardware reset register at >> physical address 0x71f00000 (system-control according to "info mtree") >> which is handled in hw/misc/slavio_misc.c. >> >> Given that SYSCTRL_SIZE isn't used at all (and the access size for >> slavio_led_mem_ops also looks wrong), the following seems to be the >> correct fix for the reset and LED access: >> >> >> --- a/hw/misc/slavio_misc.c >> +++ b/hw/misc/slavio_misc.c >> @@ -68,6 +68,7 @@ typedef struct APCState { >> } APCState; >> >> #define MISC_SIZE 1 >> +#define LED_SIZE 2 >> #define SYSCTRL_SIZE 4 >> >> #define AUX1_TC 0x02 >> @@ -452,13 +453,13 @@ static int slavio_misc_init1(SysBusDevice *sbd) >> /* 16 bit registers */ >> /* ss600mp diag LEDs */ >> memory_region_init_io(&s->led_iomem, OBJECT(s), &slavio_led_mem_ops, s, >> - "leds", MISC_SIZE); >> + "leds", LED_SIZE); >> sysbus_init_mmio(sbd, &s->led_iomem); >> >> /* 32 bit registers */ >> /* System control */ >> memory_region_init_io(&s->sysctrl_iomem, OBJECT(s), >> &slavio_sysctrl_mem_ops, s, >> - "system-control", MISC_SIZE); >> + "system-control", SYSCTRL_SIZE); >> sysbus_init_mmio(sbd, &s->sysctrl_iomem); >> >> /* AUX 1 (Misc System Functions) */ >> >> >> Then again it is getting quite late in the release cycle... > > Yes, it's better to revert for now. Can you submit the patch above, so > I include it in 2.4 and keep bisectability? Sure. Is your current 2.4 branch available on github or anywhere similar? That's just so once the patch has been applied, I have a remote I can use to run a complete set of OpenBIOS tests. ATB, Mark.
On 02/04/2015 13:45, Mark Cave-Ayland wrote: > > Yes, it's better to revert for now. Can you submit the patch above, so > > I include it in 2.4 and keep bisectability? > > Sure. Is your current 2.4 branch available on github or anywhere > similar? That's just so once the patch has been applied, I have a remote > I can use to run a complete set of OpenBIOS tests. I placed it in branch "atomic-dirty" of git://github.com/bonzini/qemu.git - for now it's only compile tested, but I'll get to it tomorrow. Thanks! Paolo
--- a/hw/misc/slavio_misc.c +++ b/hw/misc/slavio_misc.c @@ -68,6 +68,7 @@ typedef struct APCState { } APCState; #define MISC_SIZE 1 +#define LED_SIZE 2 #define SYSCTRL_SIZE 4 #define AUX1_TC 0x02 @@ -452,13 +453,13 @@ static int slavio_misc_init1(SysBusDevice *sbd) /* 16 bit registers */ /* ss600mp diag LEDs */ memory_region_init_io(&s->led_iomem, OBJECT(s), &slavio_led_mem_ops, s, - "leds", MISC_SIZE); + "leds", LED_SIZE); sysbus_init_mmio(sbd, &s->led_iomem); /* 32 bit registers */ /* System control */ memory_region_init_io(&s->sysctrl_iomem, OBJECT(s), &slavio_sysctrl_mem_ops, s, - "system-control", MISC_SIZE); + "system-control", SYSCTRL_SIZE); sysbus_init_mmio(sbd, &s->sysctrl_iomem);