| Message ID | 1371674435-14973-10-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 19.06.2013, at 22:40, Anthony Liguori wrote: > The creatively named reg field is a hypervisor assigned global > identifier for a virtual device. Despite the fact that no device > is assigned a reg of 0, guests still use it to refer to early > console. > > Instead of handling this in the VTY device, handle this in the VIO > bus since this is ultimately about how devices are decoded. > > This does not produce a change in behavior since reg=0 hcalls to > non-VTY devices will still fail as gloriously as they did before > just for a different reason (invalid device instead of invalid reg). Ben, is it true that reg=0 is guaranteed to be free, regardless of the target call? Also, are there no other PAPR calls that could possibly refer to reg=0 but mean something different from the VTY device? Alex > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > hw/char/spapr_vty.c | 58 ++-------------------------------------------- > hw/ppc/spapr_vio.c | 46 ++++++++++++++++++++++++++++++++++++ > include/hw/ppc/spapr_vio.h | 2 -- > 3 files changed, 48 insertions(+), 58 deletions(-) > > diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c > index 4bac79e..45fc3ce 100644 > --- a/hw/char/spapr_vty.c > +++ b/hw/char/spapr_vty.c > @@ -86,23 +86,6 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev) > return 0; > } > > -static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg) > -{ > - VIOsPAPRDevice *sdev; > - > - sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); > - if (!sdev && reg == 0) { > - /* Hack for kernel early debug, which always specifies reg==0. > - * We search all VIO devices, and grab the vty with the lowest > - * reg. This attempts to mimic existing PowerVM behaviour > - * (early debug does work there, despite having no vty with > - * reg==0. */ > - return spapr_vty_get_default(spapr->vio_bus); > - } > - > - return sdev; > -} > - > /* Forward declaration */ > static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, > target_ulong opcode, target_ulong *args) > @@ -114,7 +97,7 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, > VIOsPAPRDevice *sdev; > uint8_t buf[16]; > > - sdev = vty_lookup(spapr, reg); > + sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); > if (!sdev) { > return H_PARAMETER; > } > @@ -141,7 +124,7 @@ static target_ulong h_get_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, > VIOsPAPRDevice *sdev; > uint8_t buf[16]; > > - sdev = vty_lookup(spapr, reg); > + sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); > if (!sdev) { > return H_PARAMETER; > } > @@ -191,43 +174,6 @@ static const TypeInfo spapr_vty_info = { > .class_init = spapr_vty_class_init, > }; > > -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus) > -{ > - VIOsPAPRDevice *sdev, *selected; > - BusChild *kid; > - > - /* > - * To avoid the console bouncing around we want one VTY to be > - * the "default". We haven't really got anything to go on, so > - * arbitrarily choose the one with the lowest reg value. > - */ > - > - selected = NULL; > - QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { > - DeviceState *iter = kid->child; > - > - /* Only look at VTY devices */ > - if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) { > - continue; > - } > - > - sdev = VIO_SPAPR_DEVICE(iter); > - > - /* First VTY we've found, so it is selected for now */ > - if (!selected) { > - selected = sdev; > - continue; > - } > - > - /* Choose VTY with lowest reg value */ > - if (sdev->reg < selected->reg) { > - selected = sdev; > - } > - } > - > - return selected; > -} > - > static void spapr_vty_register_types(void) > { > spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char); > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c > index 2c5b159..ee99eec 100644 > --- a/hw/ppc/spapr_vio.c > +++ b/hw/ppc/spapr_vio.c > @@ -77,11 +77,57 @@ static const TypeInfo spapr_vio_bus_info = { > .instance_size = sizeof(VIOsPAPRBus), > }; > > +static VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus) > +{ > + VIOsPAPRDevice *sdev, *selected; > + BusChild *kid; > + > + /* > + * To avoid the console bouncing around we want one VTY to be > + * the "default". We haven't really got anything to go on, so > + * arbitrarily choose the one with the lowest reg value. > + */ > + > + selected = NULL; > + QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { > + DeviceState *iter = kid->child; > + > + /* Only look at VTY devices */ > + if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) { > + continue; > + } > + > + sdev = VIO_SPAPR_DEVICE(iter); > + > + /* First VTY we've found, so it is selected for now */ > + if (!selected) { > + selected = sdev; > + continue; > + } > + > + /* Choose VTY with lowest reg value */ > + if (sdev->reg < selected->reg) { > + selected = sdev; > + } > + } > + > + return selected; > +} > + > VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg) > { > BusChild *kid; > VIOsPAPRDevice *dev = NULL; > > + /* Hack for kernel early debug, which always specifies reg==0. > + * We search all VIO devices, and grab the vty with the lowest > + * reg. This attempts to mimic existing PowerVM behaviour > + * (early debug does work there, despite having no vty with > + * reg==0. */ > + if (reg == 0) { > + return spapr_vty_get_default(bus); > + } > + > QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { > dev = (VIOsPAPRDevice *)kid->child; > if (dev->reg == reg) { > diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h > index 817f5ff..f55eb90 100644 > --- a/include/hw/ppc/spapr_vio.h > +++ b/include/hw/ppc/spapr_vio.h > @@ -127,8 +127,6 @@ void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev); > void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd); > void spapr_vscsi_create(VIOsPAPRBus *bus); > > -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus); > - > void spapr_vio_quiesce(void); > > #endif /* _HW_SPAPR_VIO_H */ > -- > 1.8.0 >
Alexander Graf <agraf@suse.de> writes: > On 19.06.2013, at 22:40, Anthony Liguori wrote: > >> The creatively named reg field is a hypervisor assigned global >> identifier for a virtual device. Despite the fact that no device >> is assigned a reg of 0, guests still use it to refer to early >> console. >> >> Instead of handling this in the VTY device, handle this in the VIO >> bus since this is ultimately about how devices are decoded. >> >> This does not produce a change in behavior since reg=0 hcalls to >> non-VTY devices will still fail as gloriously as they did before >> just for a different reason (invalid device instead of invalid reg). > > Ben, is it true that reg=0 is guaranteed to be free, regardless of the > target call? reg is exposed to the guest via device tree. My assumption is that the only reason this reg=0 silliness exists is for early boot code that hasn't gotten enough working yet to actually read the device tree to discover the proper reg value for the VTY. > Also, are there no other PAPR calls that could possibly refer to reg=0 > but mean something different from the VTY device? Note that if there is, it will still fail today exactly the same way as it does without this patch. We can still add work arounds to the reg lookup code to return something different for reg=0 if a different device type is being searched for. So even if that's the case, I still think this move is the right way to handle things. Regards, Anthony Liguori > > > Alex > >> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >> --- >> hw/char/spapr_vty.c | 58 ++-------------------------------------------- >> hw/ppc/spapr_vio.c | 46 ++++++++++++++++++++++++++++++++++++ >> include/hw/ppc/spapr_vio.h | 2 -- >> 3 files changed, 48 insertions(+), 58 deletions(-) >> >> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c >> index 4bac79e..45fc3ce 100644 >> --- a/hw/char/spapr_vty.c >> +++ b/hw/char/spapr_vty.c >> @@ -86,23 +86,6 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev) >> return 0; >> } >> >> -static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg) >> -{ >> - VIOsPAPRDevice *sdev; >> - >> - sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); >> - if (!sdev && reg == 0) { >> - /* Hack for kernel early debug, which always specifies reg==0. >> - * We search all VIO devices, and grab the vty with the lowest >> - * reg. This attempts to mimic existing PowerVM behaviour >> - * (early debug does work there, despite having no vty with >> - * reg==0. */ >> - return spapr_vty_get_default(spapr->vio_bus); >> - } >> - >> - return sdev; >> -} >> - >> /* Forward declaration */ >> static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, >> target_ulong opcode, target_ulong *args) >> @@ -114,7 +97,7 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, >> VIOsPAPRDevice *sdev; >> uint8_t buf[16]; >> >> - sdev = vty_lookup(spapr, reg); >> + sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); >> if (!sdev) { >> return H_PARAMETER; >> } >> @@ -141,7 +124,7 @@ static target_ulong h_get_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, >> VIOsPAPRDevice *sdev; >> uint8_t buf[16]; >> >> - sdev = vty_lookup(spapr, reg); >> + sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); >> if (!sdev) { >> return H_PARAMETER; >> } >> @@ -191,43 +174,6 @@ static const TypeInfo spapr_vty_info = { >> .class_init = spapr_vty_class_init, >> }; >> >> -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus) >> -{ >> - VIOsPAPRDevice *sdev, *selected; >> - BusChild *kid; >> - >> - /* >> - * To avoid the console bouncing around we want one VTY to be >> - * the "default". We haven't really got anything to go on, so >> - * arbitrarily choose the one with the lowest reg value. >> - */ >> - >> - selected = NULL; >> - QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { >> - DeviceState *iter = kid->child; >> - >> - /* Only look at VTY devices */ >> - if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) { >> - continue; >> - } >> - >> - sdev = VIO_SPAPR_DEVICE(iter); >> - >> - /* First VTY we've found, so it is selected for now */ >> - if (!selected) { >> - selected = sdev; >> - continue; >> - } >> - >> - /* Choose VTY with lowest reg value */ >> - if (sdev->reg < selected->reg) { >> - selected = sdev; >> - } >> - } >> - >> - return selected; >> -} >> - >> static void spapr_vty_register_types(void) >> { >> spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char); >> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c >> index 2c5b159..ee99eec 100644 >> --- a/hw/ppc/spapr_vio.c >> +++ b/hw/ppc/spapr_vio.c >> @@ -77,11 +77,57 @@ static const TypeInfo spapr_vio_bus_info = { >> .instance_size = sizeof(VIOsPAPRBus), >> }; >> >> +static VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus) >> +{ >> + VIOsPAPRDevice *sdev, *selected; >> + BusChild *kid; >> + >> + /* >> + * To avoid the console bouncing around we want one VTY to be >> + * the "default". We haven't really got anything to go on, so >> + * arbitrarily choose the one with the lowest reg value. >> + */ >> + >> + selected = NULL; >> + QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { >> + DeviceState *iter = kid->child; >> + >> + /* Only look at VTY devices */ >> + if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) { >> + continue; >> + } >> + >> + sdev = VIO_SPAPR_DEVICE(iter); >> + >> + /* First VTY we've found, so it is selected for now */ >> + if (!selected) { >> + selected = sdev; >> + continue; >> + } >> + >> + /* Choose VTY with lowest reg value */ >> + if (sdev->reg < selected->reg) { >> + selected = sdev; >> + } >> + } >> + >> + return selected; >> +} >> + >> VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg) >> { >> BusChild *kid; >> VIOsPAPRDevice *dev = NULL; >> >> + /* Hack for kernel early debug, which always specifies reg==0. >> + * We search all VIO devices, and grab the vty with the lowest >> + * reg. This attempts to mimic existing PowerVM behaviour >> + * (early debug does work there, despite having no vty with >> + * reg==0. */ >> + if (reg == 0) { >> + return spapr_vty_get_default(bus); >> + } >> + >> QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { >> dev = (VIOsPAPRDevice *)kid->child; >> if (dev->reg == reg) { >> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h >> index 817f5ff..f55eb90 100644 >> --- a/include/hw/ppc/spapr_vio.h >> +++ b/include/hw/ppc/spapr_vio.h >> @@ -127,8 +127,6 @@ void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev); >> void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd); >> void spapr_vscsi_create(VIOsPAPRBus *bus); >> >> -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus); >> - >> void spapr_vio_quiesce(void); >> >> #endif /* _HW_SPAPR_VIO_H */ >> -- >> 1.8.0 >>
On 19.06.2013, at 23:49, Anthony Liguori wrote: > Alexander Graf <agraf@suse.de> writes: > >> On 19.06.2013, at 22:40, Anthony Liguori wrote: >> >>> The creatively named reg field is a hypervisor assigned global >>> identifier for a virtual device. Despite the fact that no device >>> is assigned a reg of 0, guests still use it to refer to early >>> console. >>> >>> Instead of handling this in the VTY device, handle this in the VIO >>> bus since this is ultimately about how devices are decoded. >>> >>> This does not produce a change in behavior since reg=0 hcalls to >>> non-VTY devices will still fail as gloriously as they did before >>> just for a different reason (invalid device instead of invalid reg). >> >> Ben, is it true that reg=0 is guaranteed to be free, regardless of the >> target call? > > reg is exposed to the guest via device tree. My assumption is that the > only reason this reg=0 silliness exists is for early boot code that > hasn't gotten enough working yet to actually read the device tree to > discover the proper reg value for the VTY. So QEMU decided the numbering scheme. Can we just use reg=0 for the default VTY? Or is reg=0 considered invalid? > >> Also, are there no other PAPR calls that could possibly refer to reg=0 >> but mean something different from the VTY device? > > Note that if there is, it will still fail today exactly the same way as > it does without this patch. > > We can still add work arounds to the reg lookup code to return something > different for reg=0 if a different device type is being searched for. As mentioned in a later patch review, that's pretty much what I think would be the best way forward, yes. Unless we can just make the default VTY be reg=0. Then there's no hack needed at all ;). > > So even if that's the case, I still think this move is the right way to > handle things. Yes, but I want to grasp the scope of potential breakage. Alex
On Wed, 2013-06-19 at 23:28 +0200, Alexander Graf wrote: > On 19.06.2013, at 22:40, Anthony Liguori wrote: > > > The creatively named reg field is a hypervisor assigned global > > identifier for a virtual device. Despite the fact that no device > > is assigned a reg of 0, guests still use it to refer to early > > console. > > > > Instead of handling this in the VTY device, handle this in the VIO > > bus since this is ultimately about how devices are decoded. > > > > This does not produce a change in behavior since reg=0 hcalls to > > non-VTY devices will still fail as gloriously as they did before > > just for a different reason (invalid device instead of invalid reg). > > Ben, is it true that reg=0 is guaranteed to be free, regardless of the target call? There is no guarantee other than what qemu does... I didn't write that code so I don't know :-) But I don't see why it can't be kept free. > Also, are there no other PAPR calls that could possibly refer to reg=0 but > mean something different from the VTY device? Not that come to mind. Ben. > > Alex > > > > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > > --- > > hw/char/spapr_vty.c | 58 ++-------------------------------------------- > > hw/ppc/spapr_vio.c | 46 ++++++++++++++++++++++++++++++++++++ > > include/hw/ppc/spapr_vio.h | 2 -- > > 3 files changed, 48 insertions(+), 58 deletions(-) > > > > diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c > > index 4bac79e..45fc3ce 100644 > > --- a/hw/char/spapr_vty.c > > +++ b/hw/char/spapr_vty.c > > @@ -86,23 +86,6 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev) > > return 0; > > } > > > > -static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg) > > -{ > > - VIOsPAPRDevice *sdev; > > - > > - sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); > > - if (!sdev && reg == 0) { > > - /* Hack for kernel early debug, which always specifies reg==0. > > - * We search all VIO devices, and grab the vty with the lowest > > - * reg. This attempts to mimic existing PowerVM behaviour > > - * (early debug does work there, despite having no vty with > > - * reg==0. */ > > - return spapr_vty_get_default(spapr->vio_bus); > > - } > > - > > - return sdev; > > -} > > - > > /* Forward declaration */ > > static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, > > target_ulong opcode, target_ulong *args) > > @@ -114,7 +97,7 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, > > VIOsPAPRDevice *sdev; > > uint8_t buf[16]; > > > > - sdev = vty_lookup(spapr, reg); > > + sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); > > if (!sdev) { > > return H_PARAMETER; > > } > > @@ -141,7 +124,7 @@ static target_ulong h_get_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, > > VIOsPAPRDevice *sdev; > > uint8_t buf[16]; > > > > - sdev = vty_lookup(spapr, reg); > > + sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); > > if (!sdev) { > > return H_PARAMETER; > > } > > @@ -191,43 +174,6 @@ static const TypeInfo spapr_vty_info = { > > .class_init = spapr_vty_class_init, > > }; > > > > -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus) > > -{ > > - VIOsPAPRDevice *sdev, *selected; > > - BusChild *kid; > > - > > - /* > > - * To avoid the console bouncing around we want one VTY to be > > - * the "default". We haven't really got anything to go on, so > > - * arbitrarily choose the one with the lowest reg value. > > - */ > > - > > - selected = NULL; > > - QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { > > - DeviceState *iter = kid->child; > > - > > - /* Only look at VTY devices */ > > - if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) { > > - continue; > > - } > > - > > - sdev = VIO_SPAPR_DEVICE(iter); > > - > > - /* First VTY we've found, so it is selected for now */ > > - if (!selected) { > > - selected = sdev; > > - continue; > > - } > > - > > - /* Choose VTY with lowest reg value */ > > - if (sdev->reg < selected->reg) { > > - selected = sdev; > > - } > > - } > > - > > - return selected; > > -} > > - > > static void spapr_vty_register_types(void) > > { > > spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char); > > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c > > index 2c5b159..ee99eec 100644 > > --- a/hw/ppc/spapr_vio.c > > +++ b/hw/ppc/spapr_vio.c > > @@ -77,11 +77,57 @@ static const TypeInfo spapr_vio_bus_info = { > > .instance_size = sizeof(VIOsPAPRBus), > > }; > > > > +static VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus) > > +{ > > + VIOsPAPRDevice *sdev, *selected; > > + BusChild *kid; > > + > > + /* > > + * To avoid the console bouncing around we want one VTY to be > > + * the "default". We haven't really got anything to go on, so > > + * arbitrarily choose the one with the lowest reg value. > > + */ > > + > > + selected = NULL; > > + QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { > > + DeviceState *iter = kid->child; > > + > > + /* Only look at VTY devices */ > > + if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) { > > + continue; > > + } > > + > > + sdev = VIO_SPAPR_DEVICE(iter); > > + > > + /* First VTY we've found, so it is selected for now */ > > + if (!selected) { > > + selected = sdev; > > + continue; > > + } > > + > > + /* Choose VTY with lowest reg value */ > > + if (sdev->reg < selected->reg) { > > + selected = sdev; > > + } > > + } > > + > > + return selected; > > +} > > + > > VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg) > > { > > BusChild *kid; > > VIOsPAPRDevice *dev = NULL; > > > > + /* Hack for kernel early debug, which always specifies reg==0. > > + * We search all VIO devices, and grab the vty with the lowest > > + * reg. This attempts to mimic existing PowerVM behaviour > > + * (early debug does work there, despite having no vty with > > + * reg==0. */ > > + if (reg == 0) { > > + return spapr_vty_get_default(bus); > > + } > > + > > QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { > > dev = (VIOsPAPRDevice *)kid->child; > > if (dev->reg == reg) { > > diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h > > index 817f5ff..f55eb90 100644 > > --- a/include/hw/ppc/spapr_vio.h > > +++ b/include/hw/ppc/spapr_vio.h > > @@ -127,8 +127,6 @@ void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev); > > void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd); > > void spapr_vscsi_create(VIOsPAPRBus *bus); > > > > -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus); > > - > > void spapr_vio_quiesce(void); > > > > #endif /* _HW_SPAPR_VIO_H */ > > -- > > 1.8.0 > >
Alexander Graf <agraf@suse.de> writes: > On 19.06.2013, at 23:49, Anthony Liguori wrote: > >> Alexander Graf <agraf@suse.de> writes: >> >>> On 19.06.2013, at 22:40, Anthony Liguori wrote: >>> >>>> The creatively named reg field is a hypervisor assigned global >>>> identifier for a virtual device. Despite the fact that no device >>>> is assigned a reg of 0, guests still use it to refer to early >>>> console. >>>> >>>> Instead of handling this in the VTY device, handle this in the VIO >>>> bus since this is ultimately about how devices are decoded. >>>> >>>> This does not produce a change in behavior since reg=0 hcalls to >>>> non-VTY devices will still fail as gloriously as they did before >>>> just for a different reason (invalid device instead of invalid reg). >>> >>> Ben, is it true that reg=0 is guaranteed to be free, regardless of the >>> target call? >> >> reg is exposed to the guest via device tree. My assumption is that the >> only reason this reg=0 silliness exists is for early boot code that >> hasn't gotten enough working yet to actually read the device tree to >> discover the proper reg value for the VTY. > > So QEMU decided the numbering scheme. Can we just use reg=0 for the > default VTY? Or is reg=0 considered invalid? We expose reg= on the command line too. I think we want to exclude 0 from that interface though. I suspect libvirt is going to want to allocate IDs to ensure a stable guest interface. Regardless of what the "first" VTY is assigned as it's reg, software is going to use reg=0 to access it. Regards, Anthony Liguori >>> Also, are there no other PAPR calls that could possibly refer to reg=0 >>> but mean something different from the VTY device? >> >> Note that if there is, it will still fail today exactly the same way as >> it does without this patch. >> >> We can still add work arounds to the reg lookup code to return something >> different for reg=0 if a different device type is being searched for. > > As mentioned in a later patch review, that's pretty much what I think would be the best way forward, yes. Unless we can just make the default VTY be reg=0. Then there's no hack needed at all ;). > >> >> So even if that's the case, I still think this move is the right way to >> handle things. > > Yes, but I want to grasp the scope of potential breakage. > > > Alex
diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c index 4bac79e..45fc3ce 100644 --- a/hw/char/spapr_vty.c +++ b/hw/char/spapr_vty.c @@ -86,23 +86,6 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev) return 0; } -static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg) -{ - VIOsPAPRDevice *sdev; - - sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); - if (!sdev && reg == 0) { - /* Hack for kernel early debug, which always specifies reg==0. - * We search all VIO devices, and grab the vty with the lowest - * reg. This attempts to mimic existing PowerVM behaviour - * (early debug does work there, despite having no vty with - * reg==0. */ - return spapr_vty_get_default(spapr->vio_bus); - } - - return sdev; -} - /* Forward declaration */ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, target_ulong opcode, target_ulong *args) @@ -114,7 +97,7 @@ static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, VIOsPAPRDevice *sdev; uint8_t buf[16]; - sdev = vty_lookup(spapr, reg); + sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); if (!sdev) { return H_PARAMETER; } @@ -141,7 +124,7 @@ static target_ulong h_get_term_char(PowerPCCPU *cpu, sPAPREnvironment *spapr, VIOsPAPRDevice *sdev; uint8_t buf[16]; - sdev = vty_lookup(spapr, reg); + sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg); if (!sdev) { return H_PARAMETER; } @@ -191,43 +174,6 @@ static const TypeInfo spapr_vty_info = { .class_init = spapr_vty_class_init, }; -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus) -{ - VIOsPAPRDevice *sdev, *selected; - BusChild *kid; - - /* - * To avoid the console bouncing around we want one VTY to be - * the "default". We haven't really got anything to go on, so - * arbitrarily choose the one with the lowest reg value. - */ - - selected = NULL; - QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { - DeviceState *iter = kid->child; - - /* Only look at VTY devices */ - if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) { - continue; - } - - sdev = VIO_SPAPR_DEVICE(iter); - - /* First VTY we've found, so it is selected for now */ - if (!selected) { - selected = sdev; - continue; - } - - /* Choose VTY with lowest reg value */ - if (sdev->reg < selected->reg) { - selected = sdev; - } - } - - return selected; -} - static void spapr_vty_register_types(void) { spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char); diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c index 2c5b159..ee99eec 100644 --- a/hw/ppc/spapr_vio.c +++ b/hw/ppc/spapr_vio.c @@ -77,11 +77,57 @@ static const TypeInfo spapr_vio_bus_info = { .instance_size = sizeof(VIOsPAPRBus), }; +static VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus) +{ + VIOsPAPRDevice *sdev, *selected; + BusChild *kid; + + /* + * To avoid the console bouncing around we want one VTY to be + * the "default". We haven't really got anything to go on, so + * arbitrarily choose the one with the lowest reg value. + */ + + selected = NULL; + QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { + DeviceState *iter = kid->child; + + /* Only look at VTY devices */ + if (!object_dynamic_cast(OBJECT(iter), "spapr-vty")) { + continue; + } + + sdev = VIO_SPAPR_DEVICE(iter); + + /* First VTY we've found, so it is selected for now */ + if (!selected) { + selected = sdev; + continue; + } + + /* Choose VTY with lowest reg value */ + if (sdev->reg < selected->reg) { + selected = sdev; + } + } + + return selected; +} + VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg) { BusChild *kid; VIOsPAPRDevice *dev = NULL; + /* Hack for kernel early debug, which always specifies reg==0. + * We search all VIO devices, and grab the vty with the lowest + * reg. This attempts to mimic existing PowerVM behaviour + * (early debug does work there, despite having no vty with + * reg==0. */ + if (reg == 0) { + return spapr_vty_get_default(bus); + } + QTAILQ_FOREACH(kid, &bus->bus.children, sibling) { dev = (VIOsPAPRDevice *)kid->child; if (dev->reg == reg) { diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h index 817f5ff..f55eb90 100644 --- a/include/hw/ppc/spapr_vio.h +++ b/include/hw/ppc/spapr_vio.h @@ -127,8 +127,6 @@ void spapr_vty_create(VIOsPAPRBus *bus, CharDriverState *chardev); void spapr_vlan_create(VIOsPAPRBus *bus, NICInfo *nd); void spapr_vscsi_create(VIOsPAPRBus *bus); -VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus); - void spapr_vio_quiesce(void); #endif /* _HW_SPAPR_VIO_H */
The creatively named reg field is a hypervisor assigned global identifier for a virtual device. Despite the fact that no device is assigned a reg of 0, guests still use it to refer to early console. Instead of handling this in the VTY device, handle this in the VIO bus since this is ultimately about how devices are decoded. This does not produce a change in behavior since reg=0 hcalls to non-VTY devices will still fail as gloriously as they did before just for a different reason (invalid device instead of invalid reg). Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- hw/char/spapr_vty.c | 58 ++-------------------------------------------- hw/ppc/spapr_vio.c | 46 ++++++++++++++++++++++++++++++++++++ include/hw/ppc/spapr_vio.h | 2 -- 3 files changed, 48 insertions(+), 58 deletions(-)