Kevin Wolf <kw...@redhat.com> writes:
> Am 03.11.2021 um 23:01 hat Hao Wu geschrieben:
>> We made 3 changes to the at24c_eeprom_init function in
>> npcm7xx_boards.c:
>>
>> 1. We allow the function to take a I2CBus* as parameter. This allows
>> us to attach an EEPROM device behind an I2C mux which is not
>> possible with the old method.
>>
>> 2. We make at24c EEPROMs are backed by drives so that we can
>> specify the content of the EEPROMs.
>>
>> 3. Instead of using i2c address as unit number, This patch assigns
>> unique unit numbers for each eeproms in each board. This avoids
>> conflict in providing multiple eeprom contents with the same address.
>> In the old method if we specify two drives with the same unit number,
>> the following error will occur: `Device with id 'none85' exists`.
>>
>> Signed-off-by: Hao Wu <wuhao...@google.com>
>> ---
>> hw/arm/npcm7xx_boards.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
>> index dec7d16ae5..9121e081fa 100644
>> --- a/hw/arm/npcm7xx_boards.c
>> +++ b/hw/arm/npcm7xx_boards.c
>> @@ -126,13 +126,17 @@ static I2CBus *npcm7xx_i2c_get_bus(NPCM7xxState *soc,
>> uint32_t num)
>> return I2C_BUS(qdev_get_child_bus(DEVICE(&soc->smbus[num]), "i2c-bus"));
>> }
>>
>> -static void at24c_eeprom_init(NPCM7xxState *soc, int bus, uint8_t addr,
>> - uint32_t rsize)
>> +static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr,
>> + uint32_t rsize, int unit_number)
>> {
>> - I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus);
>> I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
>> DeviceState *dev = DEVICE(i2c_dev);
>> + DriveInfo *dinfo;
>>
>> + dinfo = drive_get(IF_OTHER, bus, unit_number);
>> + if (dinfo) {
>> + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>> + }
>
> I may be missing the history of this series, but why do we have to use a
> legacy DriveInfo here instead of adding a drive property to the board to
> make this configurable with -blockdev?
>
> Adding even more devices that can only be configured with -drive feels
> like a step backwards to me.
More like sideways.
The big unfinished part of -blockdev is configuring onboard devices with
it.
I took a stab at one instance in commit ebc29e1bea "pc: Support firmware
configuration with -blockdev", 2019-03-11. Quoting from the commit
message:
Mapping -drive if=none,... to -blockdev is a solved problem. With
if=T other than if=none, -drive additionally configures a block device
frontend. For non-onboard devices, that part maps to -device. Also a
solved problem. For onboard devices such as PC flash memory, we have
an unsolved problem.
This is actually an instance of a wider problem: our general device
configuration interface doesn't cover onboard devices. Instead, we have
a zoo of ad hoc interfaces that are much more limited. One of them is
-drive, which we'd rather deprecate, but can't until we have suitable
replacements for all its uses.
Sadly, I can't attack the wider problem today. So back to the narrow
problem.
My first idea was to reduce it to its solved buddy by using pluggable
instead of onboard devices for the flash memory. Workable, but it
requires some extra smarts in firmware descriptors and libvirt. Paolo
had an idea that is simpler for libvirt: keep the devices onboard, and
add machine properties for their block backends.
The implementation is less than straightforward, I'm afraid.
First, block backend properties are *qdev* properties. Machines can't
have those, as they're not devices. I could duplicate these qdev
properties as QOM properties, but I hate that.
More seriously, the properties do not belong to the machine, they
belong to the onboard flash devices. Adding them to the machine would
then require bad magic to somehow transfer them to the flash devices.
Fortunately, QOM provides the means to handle exactly this case: add
alias properties to the machine that forward to the onboard devices'
properties.
Properties need to be created in .instance_init() methods. For PC
machines, that's pc_machine_initfn(). To make alias properties work,
we need to create the onboard flash devices there, too. Requires
several bug fixes, in the previous commits. We also have to realize
the devices. More on that below.
The need to create onboard devices differently results in a non-trivial
refactoring. The need for keeping -drive working complicates things
further.
The "several bug fixes" took me 26 preparatory patches, plus at least
three more to fix regressions caused by one of them.
I then did the same for ARM virt in commit e0561e60f1 "hw/arm/virt:
Support firmware configuration with -blockdev", 2019-05-07. Only a few
preparatory patches, but the patch again includes non-trivial
refactoring.
Two new machines (ARM sbsa-ref and RISC-V virt) have since done it this
way from the start, both in 2019. Definitely easier than the
refactoring I did for PC machines and ARM virt.
More than a hundred drive_get() remain in some 70 files hw/. These
should all be for onboard block devices. I'm not aware of progress
since 2019.
If "replace them all by machine properties" is the way forward, we need
to get going. At the current rate of one file a year (measured
charitably), we'll be done around 2090, provided we don't add more
(we've added quite a few since I did the first replacement).
If it isn't a practical way forward, then what is?
And this is what makes me go "more like sideways". Hao Wu is doing what
the vast majority of the code does. Yes, that's not moving us forward
on this thorny set of problems. But it's hardly setting us back,
either.
Before we can ask people to help us move forward, we need to find the
way forward. I'm not sure we have.
